On 30/03/2012 16:57, Rich Burridge wrote:
On 03/30/2012 08:04 AM, Edwin Beasant wrote:
Hi all, please may I have a code review for this Userland section of
the ksh93 move to userland, PSARC case 2012/002, CR 7106955
Webrev is at:
http://jurassic.us.oracle.com/~ebeasant/webrevs/ksh_userland_move_v3/
...
These changes add the AT&T ksh93 package (ast-base) to Userland, and
provide integration with the AT&T build system.
Warning, this does have makefile changes that affect *all* userland
builds that use prep.mk. I have aimed preserve default behaviour in
all cases, however, and rebuilt the entire gate several times to test.
.../components/ksh93/Makefile:
* line 81 s/built/build/
Corrected.
* lines 93-94 where is CLEAN_PATHS used?
It's used in the prep.mk, line 119 part of the clean rule.
.../components/ksh93/developer-astdev.p5m:
* line 23: If this is a new file, then the Copyright lines needs to be:
# Copyright (c) 2012, Oracle and/or its affiliates. All rights reserved.
Corrected.
If you are saying that the file existed in the ON consolidation, then
does the Copyright line in .../components/ksh93/ksh93.p5m need to have
a START/END year pair? (There seems to be a slight inconsistency here).
I'm not certain, but all the provided manifests are substantially
different to the original ksh93 manifest, and is in a separate
consolidation. I've been thinking of these as new files.
* line 32: Have you passed the proposed package name by David (Comay)?
Personally, I feel the name needs at least one more hierarchical level,
but David should be able to advise you here.
The package names are as they are in ON currently, were originally
planned to remain that way to ease the migration (based on a package
upgrade methodology).
I also looked in the PSARC 2012/002 case, and it's not listed there.
Whatever the package name ends up being, it should be an exported
interface for that PSARC case.
The intention for the PSARC case was to record the deltas from ON, and
that this migration be treated as such, rather than to introduce and
redocument the existing ksh93 interfaces.
These package names did originate from the great renaming, but as per
above, they will remain the same as ON. The original ksh93 case 2006/550
(http://psarc.us.oracle.com/PSARC/2006/550/materials/proposal) doesnt
mention a package name either (it would have been SUNwcs)
.../components/ksh93/ksh93.p5m:
* Again, the package name isn't mentioned as an exported interface in
the PSARC case. It should be.
See above comment.
.../components/ksh93/source-demo-ksh.p5m:
* Similar concerns again for Copyright lines and package name as exported
interface in the PSARC case.
Corrected as above - considered a new file.
* Why is this in a separate package? It seems that it would be useful (and
not too expensive size-wise) to just include it in the standard ksh93.p5m
package manifest.
The idea was to preserve the parity with ON to enable the migration as
per the planned methodology with minimum fuss. This doesn't preclude the
integration of this into the main ksh93 package in the future.
.../make-rules/attpackagemake.mk:
* line 71: do you want to use $(GMAKE) here instead of just "make"? We've
seen a bug in another Userland component recently where it was picking
up the wrong make.
In this case, no - 'make' is an instruction to the bin/package system to
build the package, not a binary name. (eugh !)
.../make-rules/prep.mk:
* lines 24-25 I agree that we need to patch in sequence, but I thought that
that was what the Userland build infrastructure did. Am I wrong? Otherwise
how are the numbered patches (seen with some components) going to be
applied in numerical order? I'm guessing (from looking at lines 63-78)
that you've changed things here. Does your new method still work with all
other Userland components (and that numerical patches continue to be
applied
in the correct order for those components that use them)?
I've made the behaviour backwards compatible with the previous incarnation:
The patches were applied in the filename collation order as provided by
sort. The problem is that the original system could download multiple
tarballs, but could not:
1. Unpack and/or relocate tarballs in order - every package that uses
multiple tarballs currently manually unpacks them after downloading.
2. Patch patches after each tarball unpacking - in the AT&T case,
several tarballs and patches have to be overlaid, to form the source tree.
When PATCH_EACH_ARCHIVE is non-null, the behaviour of specifying patches
for each archive/tarball, in order is available.
Each archive will be downloaded, unpacked (optionally relocated), post
unpack rules executed and patched in turn.
* lines 39-44: I'm struggling to see what the difference is here (apart
from
you adding a comment and reformatting). Is there one?
No difference other than the comment and formatting.
* line 47/52: Is the second part of the old comment not true (or needed)
any more?
It's not needed (IMO), and wasn't really in the first place as the
makefile always defined the maximum number of download rules. Admittedly
one could add other different suffixes, elsewhere, but that would not
appear to be the way it was intended to be used.
Cheers for taking the time:
I have re-rolled the webrev with the changes mentioned above,
Edwin
--
-- Edwin Beasant
_______________________________________________
userland-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/userland-discuss
