Gentoo Archives: gentoo-portage-dev

From: Alec Warner <antarus@g.o>
To: gentoo-portage-dev@l.g.o
Subject: Re: [gentoo-portage-dev] [PATCH 1/3 v2] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909).
Date: Mon, 20 Jan 2014 02:43:50
Message-Id: CAAr7Pr8w0MFG2HdaGVW5acZPV8hqeczvjabb_oxi6UUyjGfg2Q@mail.gmail.com
In Reply to: Re: [gentoo-portage-dev] [PATCH 1/3 v2] Have repoman check if the packages to unpack rare archive formats from SRC_URI are present in DEPEND (bug #205909). by Tom Wijsman
1 On Sun, Jan 19, 2014 at 6:23 PM, Tom Wijsman <TomWij@g.o> wrote:
2
3 > On Sun, 19 Jan 2014 04:38:31 -0500
4 > Mike Frysinger <vapier@g.o> wrote:
5 >
6 > > On Friday 17 January 2014 18:03:57 Tom Wijsman wrote:
7 > > > ---
8 > >
9 > > please shorten your commit summary and move more content to the body
10 >
11 > Right, I'm used to writing this on the command line; I'll try to look
12 > into setting up an editor or a GUI (gitg, ...) to do my commit messages.
13 >
14 > > > +getNonSystemArchiveDepends.archivers = {
15 > >
16 > > it is super weird to attach to the object like this. some might even
17 > > say wrong.
18 >
19 > It a function, this makes it a static function variable.
20 >
21 > What is weird here? It works properly, what would be wrong?
22 >
23
24 It is certainly weird (as we discussed on IRC.) I've never seen anyone do
25 it in any codebase I liked.
26
27 One of the problems is that it isn't immutable, so that earlier callers can
28 mess with later callers. That is not possible in vapier's proposal, as the
29 attributes are hidden in the function code and are not visible to callers.
30
31 > move it into the class definition.
32 > > def getNonSystemArchiveDepends(fetchlist, eapi):
33 > > ...
34 > >
35 > > ARCHIVERS = {
36 > > ...
37 > > }
38 >
39 > That makes it a non-static function variable? This is a regression.
40 >
41
42 I guess I am not seeing why it must be a static function variable. Can you
43 explain?
44
45
46 >
47 > > > + re.compile('.*\.7[zZ]$'):"app-arch/p7zip",
48 > >
49 > > regexes should always use raw strings. there should also be a space
50 > > after the colon in dicts. so you want:
51 > >
52 > > re.compile(r'.*\.7[zZ]$'): 'app-arch/p7zip',
53 >
54 > Raw strings are a solution when \ forms a problem, which is not the
55 > case here. The colon has no space after it in the rest of repoman near
56 > it, hence I left it out; is there a coding standard we're trying to
57 > respect here? Can you refer it to me?
58 >
59
60 For the colon's in dicts thing:
61
62 http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Whitespace
63
64
65 >
66 > > > + re.compile('.*\.lzma$'):"app-arch/lzma-utils",
67 > > xz-utils, not lzma-utils
68 >
69 > http://dev.gentoo.org/~ulm/pms/5/pms.html
70 >
71 > Please see "unpack" in PMS "11.3.3.13 Misc Commands", quote:
72 >
73 > "lzma-compressed files (*.lzma). Ebuilds must ensure that LZMA Utils is
74 > installed."
75 >
76 > The TODO comment that had a PMS reference was near it before, but with
77 > the refactor it has since moved; I'll put back the reference to it.
78 >
79 > > > + re.compile('.*\.(bz2?|tbz2)$'):"app-arch/bzip2",
80 > > > +
81 > > > re.compile('.*\.(tar(\.(bz2?|gz|Z))?|tbz2|t[bg]z)?$'):"app-arch/tar",
82 > > > + re.compile('.*\.(gz|tar\.Z|t[bg]z|[zZ])$'):"app-arch/gzip",
83 > > > + re.compile('.*\.tar.xz$'):"app-arch/tar",
84 > >
85 > > requiring people list gzip, tar, and bzip2 is a significant policy
86 > > change (which i'm inclined to say is wrong). it needs discussion on
87 > > the dev mailing list first.
88 >
89 > Please see "unpack" in PMS "11.3.3.13 Misc Commands", example quote:
90 >
91 > "gzip-compressed tar files (*.tar.gz, *.tgz, *.tar.Z, *.tbz). Ebuilds
92 > must ensure that GNU gzip and GNU tar are installed."
93 >
94 > To spare on mail length, I don't quote the rest of that enumeration.
95 >
96
97 The @system set in gentoo will ensure these are installed. I understand the
98 wording of PMS (as the dependencies should be expressed somewhere) but in
99 general we prefer to do that in @system. For the same reason all packages
100 do not depend on glibc, or the compiler, or anything else.
101
102
103 >
104 > > > +def checkArchiveDepends(atoms, catpkg, relative_path, \
105 > > > + system_set_atoms, needed_unpack_depends, stats, fails):
106 > >
107 > > you don't need the \ there because you have paren already to gather
108 > > things.
109 > >
110 > > > + for entry in needed_unpack_depends[catpkg]:
111 > > > + if entry not in [atom.cp for atom in atoms if
112 > > > atom != "||"]:
113 > > > + stats['unpack.' + mytype + '.missing'] += 1
114 > > > + fails['unpack.' + mytype +
115 > > > '.missing'].append( \
116 > > > + relative_path + ": %s is missing
117 > > > in %s" % \
118 > > > + (entry, mytype))
119 > >
120 > > i know existing style doesn't follow it, but imo we should avoid
121 > > string concatenation. it makes the code harder to read imo.
122 > >
123 > > key = 'unpack.%s.missing' % mytype
124 > > stats[key] += 1
125 > > fails[key].append(...)
126 > >
127 > > > +def eapi_has_xz_utils(eapi):
128 > > > + return eapi not in ("0", "1", "2")
129 > >
130 > > i would use "eapi_supports_xz_archives" unless there's a pre-existing
131 > > style -mike
132 >
133 > Good idea, I'll take these last ones into account in the next version.
134 >
135 > Thank you everyone for the reviews so far.
136 >
137 > --
138 > With kind regards,
139 >
140 > Tom Wijsman (TomWij)
141 > Gentoo Developer
142 >
143 > E-mail address : TomWij@g.o
144 > GPG Public Key : 6D34E57D
145 > GPG Fingerprint : C165 AF18 AB4C 400B C3D2 ABF0 95B2 1FCD 6D34 E57D
146 >

Replies