Hi Thorsten, Thanks for the review...
Thorsten Frueauf wrote: > Hi Jonathan et al, > > - usr/src/Makefile.master > * line 1057ff, I notice that you still append the DEVELOPERUID to the > IPSPKG_BRANCH string. > Strictly speaking, if we go by the model that each developer has its > own > dedicated repository, we don't need to append the DEVELOPERUID anymore. > So you might be able to eliminate line 1058 and remove its usage > everywhere > else. OK that makes the IPS package names a lot clearer. I've refreshed the webrev. > > * since the port number will get automagically calculated, I would vote > for also including the script to start the developer repo within the > gate along your putback, which hopefully will then use the same > algorithm to start the repo on that port number. > Otherwise it might not be obvious to developers which port they are > expected > to run the repo under. Nick is planning to send out a review for that developer repo script and we'll include it in the gate. > > - usr/src/ipsdefs/Makefile.targ > * line 55ff, I see that error handling is removed since now > IPSREPO_URL is > always set. But if the developer did not setup the repo, then all > individual > pkgsend will fail. > It might be useful to check with something like mconnect(1) or > telnet(1) to verify that > indeed something is listening on the IPSREPO_URL, if not give a > meaningful > error message. Otherwise we rely on the error that pkgsend gives to > be meaningful :) The first call to pkgsend fails with a fairly reasonable error: URLError: <urlopen error (146, 'Connection refused')> No transaction ID specified in $PKG_TRANS_ID *** Error code 1 at which point dmake quits building anything else in ipsdefs. If you're running nbuild, it will log the error in the email and continue on to the next task. > > > - usr/src/tools/scripts/nbuild.ksh > * line 395ff, might still be usefull to list IPSREPO_URL with a small > description what the default repo looks like and that it is possible to > overwrite it. I removed IPSREPO_URL because it's no longer relevant to nbuild, it's just an internal Makefile variable. I agree the repo setting needs explaining, but nbuild is not the place. > > * line 1576, could be that my understanding of Clear_env() was wrong at > the time I made the change, but if we want to allow developers to > overwrite IPSREPO_URL, don't we want to list IPSREPO_URL here, since > otherwise it will get unset, and thus the default will get used? We want developers to override the setting on the command line, not in their shell environment. The purpose of Clear_env() is to unset all environment settings, other than those used to alter the internal behaviour of the nbuild script. Any VAR=VALUE settings on the command line will be added to nbuild's environment after it's run Clear_env(). Thanks Jonathan > > Greets > Thorsten > > Jonathan Mellors wrote: >> Hi Folks, >> >> I've modified the Colorado code Thorsten originally introduced to >> support sending IPS packages to an IPS repository. The purpose of the >> change is to set a sensible default for the IPSREPO_URL in >> Makefile.master. This default setting can then be used by both nbuild >> and nbmake, or overridden if required. >> >> To allow multiple developers to have their own individual IPS >> repositories on the same build server, we came up with the idea of >> using the user's uid for the port number of the IPS depot server >> "pkg.depotd(1M)". To allow developers to assign their own ports we >> have to avoid the privileged ports (1 - 1023), which leaves us with >> the range 1024 - 65535. Any uids below 1024 will have an assigned port >> number of uid+1024. Any uids above 65535 will have an assigned port >> number of uid modulo 65536, with 1024 being added if the modulo result >> is below 1024. Obviously there's a small chance that more than one >> user could be assigned the same port number, but if that happens they >> can override IPSREPO_URL on the nbuild/nbmake command line. You would >> also have to override IPSREPO_URL, if the IPS packages are to be sent >> to any other server than the one you're building on. >> >> Webrev is at: >> http://cr.opensolaris.org/~jmellors/ipsrepo/ >> >> Thanks >> Jonathan >
