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/
* lines 93-94 where is CLEAN_PATHS used?
.../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.
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).
* 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.
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.
.../components/ksh93/ksh93.p5m:
* Again, the package name isn't mentioned as an exported interface in
the PSARC case. It should be.
.../components/ksh93/source-demo-ksh.p5m:
* Similar concerns again for Copyright lines and package name as exported
interface in the PSARC case.
* 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.
.../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.
.../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)?
* lines 39-44: I'm struggling to see what the difference is here (apart from
you adding a comment and reformatting). Is there one?
* line 47/52: Is the second part of the old comment not true (or needed)
any more?
_______________________________________________
userland-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/userland-discuss
