Hi Rich,
Thank you for reviewing and all the great comments!
Here's the webrev diff from last time:
http://jurassic.us.oracle.com/net/sfwcfi/builds/apchin/pylint-upgrade/webrev-2-diffs/
And a full webrev:
http://jurassic.us.oracle.com/net/sfwcfi/builds/apchin/pylint-upgrade/webrev-2/
See my comments below.
Thanks,
April
On 04/16/12 03:54 PM, Rich Burridge wrote:
On 04/16/2012 10:30 AM, April Chin wrote:
I'm looking for a code review for my updates to pylint,
logilab-astng, and logilab-common.
These three components must be updated together, since the updated
pylint has
a runtime dependency on the new versions of logilab-common &
logilab-astng;
there is no build dependency.
I'm still waiting for approval for the license change for
logilab-astng & logilab-common,
so the current license files may not be final.
Webrev:
http://jurassic.us.oracle.com/net/sfwcfi/builds/apchin/pylint-upgrade/webrev-1/
* If there isn't already an RFE in Bugster to add unittest2 to Solaris
(so we can properly test these components), then please file one.
Yes, there's an RFE already - 7161185 add unittest2 package to Solaris
.../components/logilab-astng/Makefile
line 23: need to update Copyright line to include 2012.
Thanks...I added 2012 to the Copyright.
.../components/logilab-astng/logilab-astng-26.p5m
.../components/logilab-astng/logilab-astng-27.p5m
line 25: Is this transform correct? Are you wanting to apply the
facet.devel
facet to all files under
usr/lib/python2.6/vendor-packages/logilab/astng/test ?
Yes, that was my intention.
I believe all the files/dirs under the test dir are related to testing
logilab-astng itself.
I marked them as devel=true to address a question in the 3PSC regarding
excluding
unneeded files (e.g., test, demo files), although I don't see this facet
used for other
test files in Userland.
Per the comment from Danek (see other email thread), I should just
remove them
from the package, since as far as I can tell, they are only for testing
the component itself.
I've checked a few other distros (Fedora, Debian, Ubuntu), some of which
do not include the test files in their package.
My .p5m regular expression-fu is sucky, but don't you need to
add a wildcard at the end?
You would think the wildcard at the end *would* be needed, but it works
with
everything starting with
usr/lib/python2.6/vendor-packages/logilab/astng/test.
I'm removing the transform line now, since it's not needed.
You could also add one at the beginning
to reduce the line length. Something like:
<transform dir file link hardlink path=usr.*astng/test/.+ -> default
facet.devel true>
Check with Danek or alanc or somebody else who totally groks
this.
* Can you not add another transform to add "pkg.tmp.autopyc=false" in a
similar way, to all the files it applies too?
Yes, thanks...it looks like all these files to which
pkg.tmp.autopyc=false are under
subdirectories of test, but now I'll be removing all the test files, so
there will no longer be
any files with "pkg.tmp.autopyc=false."
.../components/logilab-astng/logilab-astng.p5m:
line 43: I've noticed that in some other Python components under
.../componments/python/ in the Userland ws, where there is a
<component name>.p5m that has dependencies upon 2.6
and 2.7 packages, that there isn't a license line in the
<component>.p5m file. That suggests you don't need one here either
(but check with Norm).
Okay, thanks...I'm asking Norm. I've left them in for now.
.../components/logilab-common/logilab-common-26.p5m
.../components/logilab-common/logilab-common-27.p5m
line 25: similar concerns as to the logilab-astng .p5m files above.
I'm removing the test files from the logilab-common as well, so these lines
referring to things under the test directory will go away.
* If you added "pkg.tmp.autopyc=false" to a variety of the test Python
files
in the logilab-astng .p5m files, shouldn't they be added here too?
In logilab-common, the *.pyc files exist in the proto area for all the
*.py files; in logilab-astng, they
don't in some cases. See explanation below for the pylint*.p5m files.
.../components/logilab-astng/logilab-astng.p5m
line 42: similar concern to line 43 in logilab-astng.p5m.
.../components/pylint/pylint-26.p5m
* Should these vendor-packages/pylint/test files have the facet.devel
facet too?
Yes, if I'm marking tests as facet.devel=true for the unit testing files
in the logilab packages,
I should do it here as well.
* Similar concerns w.r.t. "pkg.tmp.autopyc=false". Also, it seems to
be on
some files (eg. .../test/input/func_continue_not_in_loop.py), but not
others (eg. .../test/input/func_class_members.py). What's the
difference?
Whether or not *.py files were marked "pkg.tmp.autopyc=false", depends on
on what files get installed into the proto area--some *.py files have
associated *.pyc files
and some do not. So for instance, we see
file path=usr/share/doc/pylint/examples/custom_raw.py pkg.tmp.autopyc=false
because there is a custom_raw.py file in the proto area, but there was *no*
custom_raw.pyc file installed under build/prototypes.
All *.py files not marked pkg.tmp.autopyc=false in the package
manifest will automatically get an accompanying *.pyc file in the final
package,
due to transforms/autopyc.
lines 415-418 and 420-423. I'm a little confused why these are commented
out now (rather than being removed like the comment at lines 412-414
suggests
should happen if pkgdepend is now fixed). If it's not fixed, why are they
now commented out?
Yes, I should just remove these lines, instead of commenting them out,
since they are replaced by the explicit
depend lines:
425 # force a dependency on logilab-common-26 version 0.57.1
426 depend fmri=library/python-2/[email protected] type=require
427
428 # force a dependency on logilab-astng-26 version 0.23.1
429 depend fmri=library/python-2/[email protected] type=require
which are needed to ensure we pick up the newer versions of these
packages as
dependencies, rather than the older versions installed on the build
system.
Although the commented out depend lines would work once the build system
gets updated to the newer logilab packages, it's probably better to just
leave
in the dependencies with the explicit version numbers, to ensure pylint
gets the
dependencies on the correct versions.
.../components/pylint/pylint-27.p5m
* Should these vendor-packages/pylint/test files have the facet.devel
facet?
Yes, you're right. Now that I'm removing the test files, I won't need it.
* Similar concerns to pylint/pylint-26.p5m w.r.t.
"pkg.tmp.autopyc=false".
Ditto
.../components/pylint/pylint.p5m
* Similar concern to logilab-astng.p5m and logilab-astng.p5m on
whether you
need a "licence ..." line in this .p5m file.
Yes, I'll leave them in for now, until I hear otherwise.
_______________________________________________
userland-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/userland-discuss
