Hi Jack,
Thanks for looking at this, I have some comments...
General:
- Why is the file called ai_svr4_admin - I think the ai_ should be omitted
since it's not unique to AI and could just as easily be used in a DC build,
I feel it would suffice to be svr4_admin since it's installed in
/usr/share/install.
File svr4.py:
- General
It looks like the package datastream may be opened at least 3 times,
specifically I feel this is excessive if we're using a remote URL:
1) In get_pkgurl_status() to just see if it's remote or not, and whether it's
a datastream or not - I'm not sure how much is actually downloaded in this
case - if it's small then it may acceptable.
2) In ds_pkg_size_and_verify() it's opened again, again to see if it's remote
or not, or a datastream or not
This time it's also fully streamed down from a remote site to see what
size it is, but we don't save this stream anywhere.
3) Finally, we call the pkgadd command with the URL again, which in turn
downloads the stream again.
I really feel that we could do some optimizing here, and only download the
stream once if it's a remote stream - this could be facilitated by
a method that encapsulates the urlopen() that does the full streaming (2
above) and does it only once caching to a file - subsequent calls would open
the cached file, and the location of this would be what's passed to the
pkgadd command.
Maybe there is not any real need to download the whole stream in our code,
but just look at the HTTP header which should be able to tell you the size of
the datastream - but this may not always be there, so we should also handle
this by returning an 'indeterminate' size. It's only an informational
message, and we shouldn't really need to download a possibly large file just
to get the information.
- lines 116-122:
These might be better placed as part of an if/else is_remote_file at lines
126-127 - it's confusing to be opening the remote URL earlier while waiting
to open a local file URL.
(i.e. Looking at line 130, the first question is where is fd being opened if
it's not a local file?)
Thanks,
Darren.
On 01/09/2011 09:52, Jack Schwartz wrote:
> Hi everyone.
>
> Here is a code review to fix:
> 7084807 <http://monaco.us.oracle.com/detail.jsf?cr=7084807> SVR4 package
> installs using the new autoinstaller don't work
>
> Webrev:
> https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_1
>
> Bug report:
> http://monaco.us.oracle.com/detail.jsf?cr=7084807
>
> Please review and bless by Friday 9/2 10 AM PST.
>
> I have done a lot of standalone testing with a special setup. Testing is
> still
> in progress and is looking good. I want to get the webrev out before I
> complete
> testing so I can get this into B174, pushing Friday COB. I don't anticipate
> any
> major changes at this point.
>
> Testing has included remote and local datastream files with 1 and >1 packages
> requested, local directories which are parents to groups of package trees.
>
> Testing still to be done:
> - A full AI install. Up until now I have bypassed IPS package installs to
> save
> time. I'll do this first thing in the morning.
> - Unit tests still need a little more work.
>
> Note: I introduce an SVR4 admin file to AI client /usr/share/install directory
> with this push. It is needed for non-interactive SVR4 installs.
>
> Thanks,
> Jack
>
>
> _______________________________________________
> caiman-discuss mailing list
> [email protected]
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
