Hi Jack,
Thanks for the review.
On 02/05/2011 23:50, Jack Schwartz wrote:
> Hi Darren and Matt.
>
> Here is part of of my AI-CUD review. It does not include tests. Also it does
> not include target_selection.py as I saved the best for last :) I plan on
> doing
> target_selection.py tomorrow...
>
> I hope this isn't too late. I got stuck last week with last minute urgent
> Derived Manifest stuff...
>
Not an issue at all :)
>
> publish_manifest.py:
> --------------------
> OK sans DMM changes*
>
> Makefile.targ:
> --------------
> OK sans DMM changes*
>
> *AI-server changes I made for Derived Manifests project went back to
> slim_source
> later and so aren't reflected here.
>
> auto-install/Makefile:
> ----------------------
> Is there a .po file which gets built? Building of the .po was removed from
> this
> Makefile. Is it elsewhere?
It's not anywhere - since all the .c and .h files were removed, there is no
.po being generated
>
> ai_get_manifest.py:
> -------------------
> 49-50: This kind of thing is good to have defined in a common file as it is
> used
> all over. (I moved this into install_common in my push. You may consider
> using
> it from there.)
Interstingly, it is already importing it from there, so this is duplicate.
>
> 73, 828, 829: While on the subject of common stuff, I suggest moving a
> definition of /var/run to install_common as well so it is not duplicated, then
> reference that definition from everywhere. That way if it changes again, you
> need to change it only in one place.
It's a good suggestion, but I think a general sweep of slim_source should be
done too since there is quite lot of references to /tmp still in there, the
problem isn't just in python code either, there are shell scripts (e.g. the
manifest-locator) that need synchronising too.
I've added two variables, and utility methods to install_common/__init__.py
that can be used here:
system_temp_path( <file> )
will return /system/volatile/<file>, or just /system/volatile
if no file given.
post_install_logs_path( <file> )
will return /var/sadm/systen/logs/<file> or just the dir if no
path given.
I updated the cmd/auto-install based sources for these.
>
> ai_sd.py:
> ---------
> ai_get_manifest lines 73, 828, 829
Fixed references to be /system/volatile.
>
> auto_install.py:
> ---------------
> 188, 225: It may be clearer to call "profile" as in options.profile something
> other than "profile" as one may confuse that with SC profiles. What do you
> think?
Yes, I think so. Dave suggested we follow the AI/Zones project's proposal and
use -m for the manifest rather than -p, which will be used by that project for
the SCI profiles.
>
> 236-238 and 243: this can be replaced with
> fl = linecache.getline(profile, 1)
> See dmm.py for an example. Thanks to Drew for pointing this out during DM
> code
> review.
He missed it in this one ;)
Changed...
>
> 490: Nit: successful is spelled with one ell.
Fixed.
>
> 527, 545: I don't understand the logic here. Why are these messages printed
> only when there are no checkpoints?
It's not when there are no checkpoints, its when auto-install has been called
with the -l option to 'list checkpoints'. Not printing the messages makes the
output of -l cleaner.
>
> 850, 855, 864, 867, 870, 878, others?: set up another shared definition in
> install_common for /var/sadm/system. Use the one proposed earlier for
> /var/run.
Done, but I didn't define anything for /var/sadm/system just
/var/sadm/system/logs - so far I've not found anythin that uses it without the
logs part.
>
> 956: What do 10000 and 30000 represent? Please add a comment.
Just the starting and end points of a random range to pick.
>
> 1011, 1012: What does 524288 represent? Please add a comment.
As I said to Dave, I honestly don't know - this is something we got from the
logger tests.
>
>
> svc/Makefile:
> svc/auto-installer.xml:
> ----------------------
> No changes other than copyright. Suggest deleting from review and push.
Done
>
> manifest-locator:
> -----------------
> Continuing with common definitions implementation, if /var/run/ai.xml is
> defined
> in in install_common like this:
> AI_XML="/var/run/ai.xml"
> you could replace in manifest-locator
> AI_MANIFEST=/var/run/ai.xml
> with
> AI_MANIFEST=`python -c "import solaris_install.install_common; print
> AI_XML"`
>
> 66: similarly for AI_SERVICE_LIST
>
> Other defs too?
I think this is another good idea, but we've only minimally changed these, so
I think it would be best done as part of an overall sweep of things.
I could do it, if there were suggestions on where to put such values so that
we could pick them up in scripts.
>
> utmpx.py:
> ---------
> 58: Too bad there isn't a way of passing pid_t to something which
> magically translates it to a uint. Else if it changes and breaks something,
> it
> will be a nightmare to figure it out...
If this changes considerably at any time, I would agree - but honestly I think
it's a public interface, so unlikely to change too quickly - if they do a grep
will find the references.
>
> users_on_console() and the main look remarkably similar. Can they be
> combined?
Done, the main was primarily a test.
>
> installadm/__init__.py:
> installadm/Makefile:
> -----------------------
> Just curious: why is __init__.py needed?
At one point installadm was expecting auto-install to put this find in place
in osol_install/auto_install - but of course we're not delivering in there, so
without this file installadm would break.
It's not really part of the scope of this project to change installadm.
>
> install_common/__init__.py:
> ---------------------------
> 370: Defer to previous comments about shared paths and other definitions.
Done.
>
> ai.dtd:
> -------
> This appears to be an artifact of previous gate state (pre-SC-profiles) and is
> no longer relevant. Is this true?
As far as I know, yes. The only DTDs that should be left are the new ones - DC
is already in place, and AI is the last DTD to move.
>
> system-install-auto-install.mf: which package do the DTDS go back with now?
> They have been removed from here, and not added to the other pkg manifest of
> this code review.
The are already being delivered in slim_source in the system-library-install
package.
Thanks,
Darren.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
