Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6576023:
scripts: call devserver/payload generators from chroot & delete moved scripts (Closed)
|
Created:
9 years, 10 months ago by zbehan Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Visibility:
Public. |
Descriptionscripts: call devserver/payload generators from chroot & delete moved scripts
BUG=chromium-os:5246
TEST=below, During all tests, the directory platform/dev was manually removed
1) Ran cros_au_test_harness.py and saw it succeed
2) Ran generate_au_zip.py, compared the generated .zip with one before this
patch - they were identical
3) Ran cros_image_to_target.py and saw it succeed
Change-Id: Iab2620245a45442b79ee8369f3af1a3990f4644e
Patch Set 1 #
Total comments: 1
Patch Set 2 : Possible fix for calling image_to_target from outside the chroot #
Total comments: 5
Patch Set 3 : start_devserver pwn #
Total comments: 2
Patch Set 4 : Keep bits of old start_devserver #
Total comments: 1
Messages
Total messages: 31 (0 generated)
LGTM for the generate-au-zip part.
http://codereview.chromium.org/6576023/diff/1/bin/cros_image_to_target.py File bin/cros_image_to_target.py (right): http://codereview.chromium.org/6576023/diff/1/bin/cros_image_to_target.py#new... bin/cros_image_to_target.py:209: if not self.cmd.Run('cros_generate_update_payload', These changes force the user to run from within the chroot, whereas the CrosUtilsPath indirection was meant to prevent that from necessarily being the case. Perhaps other changes have broken cros_image_to_target outside the chroot though...
Yes, you're right. These are actually very likely breaking running outside the chroot, and something I haven't thought of. While I was aware that image_to_live.sh is required to run both inside and outside (and running it outside is part of au_harness), I didn't realise that this script is also intended to be ran both inside and outside. I'll address that. As a separate thing that puzzles me, I understand cros_image_to_target tries to be a more sophisticated and robust replacement for image_to_live, wouldn't it be better to decide on using just one and port all the locations to use it? Keeping two scripts around that do essentially the same seems like a lot of unnecessary maintenance. On Thu, Feb 24, 2011 at 7:49 PM, <pstew@chromium.org> wrote: > > http://codereview.chromium.org/6576023/diff/1/bin/cros_image_to_target.py > File bin/cros_image_to_target.py (right): > > > http://codereview.chromium.org/6576023/diff/1/bin/cros_image_to_target.py#new... > bin/cros_image_to_target.py:209: if not > self.cmd.Run('cros_generate_update_payload', > These changes force the user to run from within the chroot, whereas the > CrosUtilsPath indirection was meant to prevent that from necessarily > being the case. Perhaps other changes have broken cros_image_to_target > outside the chroot though... > > > http://codereview.chromium.org/6576023/ >
On Thu, Feb 24, 2011 at 11:36 AM, Zdenek Behan <zbehan@chromium.org> wrote: > Yes, you're right. These are actually very likely breaking running outside > the chroot, and something I haven't thought of. While I was aware that > image_to_live.sh is required to run both inside and outside (and running it > outside is part of au_harness), I didn't realise that this script is also > intended to be ran both inside and outside. I'll address that. > As a separate thing that puzzles me, I understand cros_image_to_target tries > to be a more sophisticated and robust replacement for image_to_live, image_to_target achieves a similar end to image_to_live, except for a couple constraints specific to installing images on devices-under-test in the WiFi testbed and other places where there's only unidirectional TCP SYN connectivity enforced by firewall. This latter case already requires a few tricks that image_to_live doesn't have (and in the past when trying to use it, I wasn't able to keep in sync with other changes to devserver and image_to_live). image_to_target strives to be relatively stable and standalone, since we have a relatively stable use case, while image_to_live (and the devserver beneath it) have historically moved quickly and in lock-step with each other, leaving previous tools I've written behind. I've had a good run with image_to_target so far, so absent a need to permanently support our use case (in a maintained manner) in image_to_live, I think we're more or less okay like this. > wouldn't it be better to decide on using just one and port all the locations > to use it? Keeping two scripts around that do essentially the same seems > like a lot of unnecessary maintenance. Actually, since the split, my maintenance costs (fixing things so our case works again when underlying stuff gets re-architected) have gone down dramatically. With the only dependence being on the image creation utilities and the actual update protocol, the rate of breakage has been so much more manageable. -- Paul > On Thu, Feb 24, 2011 at 7:49 PM, <pstew@chromium.org> wrote: >> >> http://codereview.chromium.org/6576023/diff/1/bin/cros_image_to_target.py >> File bin/cros_image_to_target.py (right): >> >> >> http://codereview.chromium.org/6576023/diff/1/bin/cros_image_to_target.py#new... >> bin/cros_image_to_target.py:209: if not >> self.cmd.Run('cros_generate_update_payload', >> These changes force the user to run from within the chroot, whereas the >> CrosUtilsPath indirection was meant to prevent that from necessarily >> being the case. Perhaps other changes have broken cros_image_to_target >> outside the chroot though... >> >> http://codereview.chromium.org/6576023/ > >
From my outsider POV, it would seem that to keep breakage down, you need proper test cases and running them with every build, rather than having a separate instance of the script that noone touches and where you roll in the changes yourself. Of course, I understand that is much easier to say than do, and I don't know the background around this either, so I'm not going to dig in it more than is necessary. But the build system improvements we are working on may actually help with this, at least in the future. Anyway, it seems that running this outside the chroot has already been broken by gspencer's CL some weeks back, because the first thing cros_generate_update_payload does is assert_inside_chroot. That happened because chromeos-common.sh has moved. This could in theory by worked around, I already had to do it because of factory in one place (they were the only ones to complain). That said, we'd like to keep things as much inside chroot as possible, and I don't think we want to support the use case of running outside unless it's strictly necessary (and even then at least try to make it not require it). Fixing image_to_target itself to run outside the chroot is trivial as you already provide a ChrootPath() function, so I can include it in this CL, but without the above change wrt chromeos-common.sh, this is entirely pointless. On Thu, Feb 24, 2011 at 9:10 PM, Paul Stewart <pstew@chromium.org> wrote: > On Thu, Feb 24, 2011 at 11:36 AM, Zdenek Behan <zbehan@chromium.org> > wrote: > > Yes, you're right. These are actually very likely breaking running > outside > > the chroot, and something I haven't thought of. While I was aware that > > image_to_live.sh is required to run both inside and outside (and running > it > > outside is part of au_harness), I didn't realise that this script is also > > intended to be ran both inside and outside. I'll address that. > > As a separate thing that puzzles me, I understand cros_image_to_target > tries > > to be a more sophisticated and robust replacement for image_to_live, > > image_to_target achieves a similar end to image_to_live, except for a > couple constraints specific to installing images on devices-under-test > in the WiFi testbed and other places where there's only unidirectional > TCP SYN connectivity enforced by firewall. This latter case already > requires a few tricks that image_to_live doesn't have (and in the past > when trying to use it, I wasn't able to keep in sync with other > changes to devserver and image_to_live). image_to_target strives to > be relatively stable and standalone, since we have a relatively stable > use case, while image_to_live (and the devserver beneath it) have > historically moved quickly and in lock-step with each other, leaving > previous tools I've written behind. I've had a good run with > image_to_target so far, so absent a need to permanently support our > use case (in a maintained manner) in image_to_live, I think we're more > or less okay like this. > > > wouldn't it be better to decide on using just one and port all the > locations > > to use it? Keeping two scripts around that do essentially the same seems > > like a lot of unnecessary maintenance. > > Actually, since the split, my maintenance costs (fixing things so our > case works again when underlying stuff gets re-architected) have gone > down dramatically. With the only dependence being on the image > creation utilities and the actual update protocol, the rate of > breakage has been so much more manageable. > > -- > Paul > > > On Thu, Feb 24, 2011 at 7:49 PM, <pstew@chromium.org> wrote: > >> > >> > http://codereview.chromium.org/6576023/diff/1/bin/cros_image_to_target.py > >> File bin/cros_image_to_target.py (right): > >> > >> > >> > http://codereview.chromium.org/6576023/diff/1/bin/cros_image_to_target.py#new... > >> bin/cros_image_to_target.py:209: if not > >> self.cmd.Run('cros_generate_update_payload', > >> These changes force the user to run from within the chroot, whereas the > >> CrosUtilsPath indirection was meant to prevent that from necessarily > >> being the case. Perhaps other changes have broken cros_image_to_target > >> outside the chroot though... > >> > >> http://codereview.chromium.org/6576023/ > > > > >
Changed cros_image_to_target to use properly self.ChrootPath. This would theoretically allow calling from outside the chroot, given a small change in cros_generate_update_payload. I have LGTM from Raja to his part. Any other comments? dale, is the testing lab updated to survive the impact of this? pstew, you ok with the modification like this? sosa, you want to check out as the last CL in devserver series? This is generally a CL with big impact, so the more eyes, the better. On Tue, Mar 1, 2011 at 3:24 AM, <zbehan@chromium.org> wrote: > http://codereview.chromium.org/6576023/ >
On 2011/03/01 02:28:19, zbehan wrote: > Changed cros_image_to_target to use properly self.ChrootPath. This would > theoretically allow calling from outside the chroot, given a small change in > cros_generate_update_payload. > > I have LGTM from Raja to his part. Any other comments? > dale, is the testing lab updated to survive the impact of this? > pstew, you ok with the modification like this? > sosa, you want to check out as the last CL in devserver series? > > This is generally a CL with big impact, so the more eyes, the better. > > On Tue, Mar 1, 2011 at 3:24 AM, <mailto:zbehan@chromium.org> wrote: > > > http://codereview.chromium.org/6576023/ > > LGTM from me, although I have seldom used this script. Pleased to see this change, thanks.
http://codereview.chromium.org/6576023/diff/7001/bin/cros_image_to_target.py File bin/cros_image_to_target.py (right): http://codereview.chromium.org/6576023/diff/7001/bin/cros_image_to_target.py#... bin/cros_image_to_target.py:209: if not self.cmd.Run(self.ChrootPath('/usr/bin/cros_generate_update_payload'), Is /usr/bin not in the path already? http://codereview.chromium.org/6576023/diff/7001/bin/cros_image_to_target.py#... bin/cros_image_to_target.py:225: self.ChrootPath('/usr/bin/cros_generate_stateful_update_payload'), Same comment
You should modify start_devserver in crosutils to call start_devserver in chroot if not --archive_dir and after restart_in_chroot_if_needed ... this way the src/platform/dev call will not be used except for outside the chroot usage. http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh File image_to_live.sh (right): http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh#newcode225 image_to_live.sh:225: if ! local stateful_update_script="$(./enter_chroot.sh \ Why is the which check necessary? We don't check for start_devserver so we shouldn't check for the other.
LGTM, modulo tests quoted are still current with latest changes. http://codereview.chromium.org/6576023/diff/7001/bin/cros_image_to_target.py File bin/cros_image_to_target.py (right): http://codereview.chromium.org/6576023/diff/7001/bin/cros_image_to_target.py#... bin/cros_image_to_target.py:209: if not self.cmd.Run(self.ChrootPath('/usr/bin/cros_generate_update_payload'), On 2011/03/01 05:04:51, sjg wrote: > Is /usr/bin not in the path already? It's not in the path if run outside the chroot. http://codereview.chromium.org/6576023/diff/7001/bin/cros_image_to_target.py#... bin/cros_image_to_target.py:225: self.ChrootPath('/usr/bin/cros_generate_stateful_update_payload'), On 2011/03/01 05:04:51, sjg wrote: > Same comment Same response.
That is not how self.ChrootPath works. It ignores PATH, just appends path into chroot before the argument (if there's anything to append). On Tue, Mar 1, 2011 at 6:04 AM, <sjg@chromium.org> wrote: > On 2011/03/01 02:28:19, zbehan wrote: > >> Changed cros_image_to_target to use properly self.ChrootPath. This would >> theoretically allow calling from outside the chroot, given a small change >> in >> cros_generate_update_payload. >> > > I have LGTM from Raja to his part. Any other comments? >> dale, is the testing lab updated to survive the impact of this? >> pstew, you ok with the modification like this? >> sosa, you want to check out as the last CL in devserver series? >> > > This is generally a CL with big impact, so the more eyes, the better. >> > > On Tue, Mar 1, 2011 at 3:24 AM, <mailto:zbehan@chromium.org> wrote: >> > > > http://codereview.chromium.org/6576023/ >> > >> > > LGTM from me, although I have seldom used this script. Pleased to see this > change, thanks. > > > > http://codereview.chromium.org/6576023/ >
I'm actually planning to get rid of platform/dev entirely (though this CL is not yet final for that). On Tue, Mar 1, 2011 at 6:40 PM, <sosa@chromium.org> wrote: > You should modify start_devserver in crosutils to call start_devserver in > chroot > if not --archive_dir and after restart_in_chroot_if_needed ... this way the > src/platform/dev call will not be used except for outside the chroot usage. > > > http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh > File image_to_live.sh (right): > > > http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh#newcode225 > image_to_live.sh:225: if ! local > stateful_update_script="$(./enter_chroot.sh \ > Why is the which check necessary? We don't check for start_devserver so > we shouldn't check for the other. Let me turn it around. We're checking for this one so we should also check for start_devserver. Using which grants wider path independence.
On Tue, Mar 1, 2011 at 10:38 AM, Zdenek Behan <zbehan@chromium.org> wrote: > I'm actually planning to get rid of platform/dev entirely (though this CL is > not yet final for that). I was thinking that this CL pretty much eliminates the use of the use of platform/dev/devserver code path except for use of start_devserver. If you remove the reference in that script, you can send out an email to chromiumos-dev to notify devs that changes to the devserver requires working on it. Otherwise, it depends if you use start_devsever or not. > > On Tue, Mar 1, 2011 at 6:40 PM, <sosa@chromium.org> wrote: >> >> You should modify start_devserver in crosutils to call start_devserver in >> chroot >> if not --archive_dir and after restart_in_chroot_if_needed ... this way >> the >> src/platform/dev call will not be used except for outside the chroot >> usage. >> >> >> http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh >> File image_to_live.sh (right): >> >> >> http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh#newcode225 >> image_to_live.sh:225: if ! local >> stateful_update_script="$(./enter_chroot.sh \ >> Why is the which check necessary? We don't check for start_devserver so >> we shouldn't check for the other. > > Let me turn it around. We're checking for this one so we should also check > for start_devserver. Using which grants wider path independence. I'm not sure you should check for it because it'll die anyway saying path can't be found. I'm not sure adding a Die with a re-wording will help much. The code should already be set -e
On Tue, Mar 1, 2011 at 8:57 PM, Chris Sosa <sosa@chromium.org> wrote: > On Tue, Mar 1, 2011 at 10:38 AM, Zdenek Behan <zbehan@chromium.org> wrote: > > I'm actually planning to get rid of platform/dev entirely (though this CL > is > > not yet final for that). > > I was thinking that this CL pretty much eliminates the use of the use > of platform/dev/devserver code path except for use of start_devserver. > If you remove the reference in that script, you can send out an email > to chromiumos-dev to notify devs that changes to the devserver > requires working on it. Otherwise, it depends if you use > start_devsever or not. > I believe that start_devserver already is eliminated. It should be used exclusively from chroot by now. In fact the things that now explicitly depend on platform/dev/ don't have much/anything to do with devserver. Namely gmergefs and factory scripts (which are up for surgery next). I just didn't delete it from src/scripts/ yet, but I scrubbed every use of it i found to call the chroot installation instead. > > > On Tue, Mar 1, 2011 at 6:40 PM, <sosa@chromium.org> wrote: > >> > >> You should modify start_devserver in crosutils to call start_devserver > in > >> chroot > >> if not --archive_dir and after restart_in_chroot_if_needed ... this way > >> the > >> src/platform/dev call will not be used except for outside the chroot > >> usage. > >> > >> > >> http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh > >> File image_to_live.sh (right): > >> > >> > >> > http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh#newcode225 > >> image_to_live.sh:225: if ! local > >> stateful_update_script="$(./enter_chroot.sh \ > >> Why is the which check necessary? We don't check for start_devserver so > >> we shouldn't check for the other. > > > > Let me turn it around. We're checking for this one so we should also > check > > for start_devserver. Using which grants wider path independence. > > I'm not sure you should check for it because it'll die anyway saying > path can't be found. I'm not sure adding a Die with a re-wording will > help much. The code should already be set -e > OK. That makes sense, I'll re-hardcode it to /usr/bin/.
On Tue, Mar 1, 2011 at 12:09 PM, Zdenek Behan <zbehan@chromium.org> wrote: > > > On Tue, Mar 1, 2011 at 8:57 PM, Chris Sosa <sosa@chromium.org> wrote: >> >> On Tue, Mar 1, 2011 at 10:38 AM, Zdenek Behan <zbehan@chromium.org> wrote: >> > I'm actually planning to get rid of platform/dev entirely (though this >> > CL is >> > not yet final for that). >> >> I was thinking that this CL pretty much eliminates the use of the use >> of platform/dev/devserver code path except for use of start_devserver. >> If you remove the reference in that script, you can send out an email >> to chromiumos-dev to notify devs that changes to the devserver >> requires working on it. Otherwise, it depends if you use >> start_devsever or not. > > I believe that start_devserver already is eliminated. It should be used > exclusively from chroot by now. In fact the things that now explicitly > depend on platform/dev/ don't have much/anything to do with devserver. > Namely gmergefs and factory scripts (which are up for surgery next). I just > didn't delete it from src/scripts/ yet, but I scrubbed every use of it i > found to call the chroot installation instead. start_devserver for many dev workflows is a first-level called script. That's the issue ... it's not other calling scripts but devs invoking it directly. >> >> > >> > On Tue, Mar 1, 2011 at 6:40 PM, <sosa@chromium.org> wrote: >> >> >> >> You should modify start_devserver in crosutils to call start_devserver >> >> in >> >> chroot >> >> if not --archive_dir and after restart_in_chroot_if_needed ... this way >> >> the >> >> src/platform/dev call will not be used except for outside the chroot >> >> usage. >> >> >> >> >> >> http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh >> >> File image_to_live.sh (right): >> >> >> >> >> >> >> >> http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh#newcode225 >> >> image_to_live.sh:225: if ! local >> >> stateful_update_script="$(./enter_chroot.sh \ >> >> Why is the which check necessary? We don't check for start_devserver >> >> so >> >> we shouldn't check for the other. >> > >> > Let me turn it around. We're checking for this one so we should also >> > check >> > for start_devserver. Using which grants wider path independence. >> >> I'm not sure you should check for it because it'll die anyway saying >> path can't be found. I'm not sure adding a Die with a re-wording will >> help much. The code should already be set -e > > OK. That makes sense, I'll re-hardcode it to /usr/bin/.
On Tue, Mar 1, 2011 at 9:10 PM, Chris Sosa <sosa@chromium.org> wrote: > On Tue, Mar 1, 2011 at 12:09 PM, Zdenek Behan <zbehan@chromium.org> wrote: > > > > > > On Tue, Mar 1, 2011 at 8:57 PM, Chris Sosa <sosa@chromium.org> wrote: > >> > >> On Tue, Mar 1, 2011 at 10:38 AM, Zdenek Behan <zbehan@chromium.org> > wrote: > >> > I'm actually planning to get rid of platform/dev entirely (though this > >> > CL is > >> > not yet final for that). > >> > >> I was thinking that this CL pretty much eliminates the use of the use > >> of platform/dev/devserver code path except for use of start_devserver. > >> If you remove the reference in that script, you can send out an email > >> to chromiumos-dev to notify devs that changes to the devserver > >> requires working on it. Otherwise, it depends if you use > >> start_devsever or not. > > > > I believe that start_devserver already is eliminated. It should be used > > exclusively from chroot by now. In fact the things that now explicitly > > depend on platform/dev/ don't have much/anything to do with devserver. > > Namely gmergefs and factory scripts (which are up for surgery next). I > just > > didn't delete it from src/scripts/ yet, but I scrubbed every use of it i > > found to call the chroot installation instead. > > start_devserver for many dev workflows is a first-level called script. > That's the issue ... it's not other calling scripts but devs invoking > it directly. > So you're saying i shouldn't delete it, and instead make the scripts/ start_devserver call the chroot start_devserver? >> > >> > > >> > On Tue, Mar 1, 2011 at 6:40 PM, <sosa@chromium.org> wrote: > >> >> > >> >> You should modify start_devserver in crosutils to call > start_devserver > >> >> in > >> >> chroot > >> >> if not --archive_dir and after restart_in_chroot_if_needed ... this > way > >> >> the > >> >> src/platform/dev call will not be used except for outside the chroot > >> >> usage. > >> >> > >> >> > >> >> http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh > >> >> File image_to_live.sh (right): > >> >> > >> >> > >> >> > >> >> > http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh#newcode225 > >> >> image_to_live.sh:225: if ! local > >> >> stateful_update_script="$(./enter_chroot.sh \ > >> >> Why is the which check necessary? We don't check for start_devserver > >> >> so > >> >> we shouldn't check for the other. > >> > > >> > Let me turn it around. We're checking for this one so we should also > >> > check > >> > for start_devserver. Using which grants wider path independence. > >> > >> I'm not sure you should check for it because it'll die anyway saying > >> path can't be found. I'm not sure adding a Die with a re-wording will > >> help much. The code should already be set -e > > > > OK. That makes sense, I'll re-hardcode it to /usr/bin/. >
On Tue, Mar 1, 2011 at 12:12 PM, Zdenek Behan <zbehan@chromium.org> wrote: > > > On Tue, Mar 1, 2011 at 9:10 PM, Chris Sosa <sosa@chromium.org> wrote: >> >> On Tue, Mar 1, 2011 at 12:09 PM, Zdenek Behan <zbehan@chromium.org> wrote: >> > >> > >> > On Tue, Mar 1, 2011 at 8:57 PM, Chris Sosa <sosa@chromium.org> wrote: >> >> >> >> On Tue, Mar 1, 2011 at 10:38 AM, Zdenek Behan <zbehan@chromium.org> >> >> wrote: >> >> > I'm actually planning to get rid of platform/dev entirely (though >> >> > this >> >> > CL is >> >> > not yet final for that). >> >> >> >> I was thinking that this CL pretty much eliminates the use of the use >> >> of platform/dev/devserver code path except for use of start_devserver. >> >> If you remove the reference in that script, you can send out an email >> >> to chromiumos-dev to notify devs that changes to the devserver >> >> requires working on it. Otherwise, it depends if you use >> >> start_devsever or not. >> > >> > I believe that start_devserver already is eliminated. It should be used >> > exclusively from chroot by now. In fact the things that now explicitly >> > depend on platform/dev/ don't have much/anything to do with devserver. >> > Namely gmergefs and factory scripts (which are up for surgery next). I >> > just >> > didn't delete it from src/scripts/ yet, but I scrubbed every use of it i >> > found to call the chroot installation instead. >> >> start_devserver for many dev workflows is a first-level called script. >> That's the issue ... it's not other calling scripts but devs invoking >> it directly. > > So you're saying i shouldn't delete it, and instead make the scripts/ > start_devserver call the chroot start_devserver? Well if you're not ready to delete in this CL, then yes, it seems to make sense for it to invoke the chroot devserver so that all uses of the devserver in the chroot go to the right place. >> >> >> >> >> > >> >> > On Tue, Mar 1, 2011 at 6:40 PM, <sosa@chromium.org> wrote: >> >> >> >> >> >> You should modify start_devserver in crosutils to call >> >> >> start_devserver >> >> >> in >> >> >> chroot >> >> >> if not --archive_dir and after restart_in_chroot_if_needed ... this >> >> >> way >> >> >> the >> >> >> src/platform/dev call will not be used except for outside the chroot >> >> >> usage. >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh >> >> >> File image_to_live.sh (right): >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh#newcode225 >> >> >> image_to_live.sh:225: if ! local >> >> >> stateful_update_script="$(./enter_chroot.sh \ >> >> >> Why is the which check necessary? We don't check for >> >> >> start_devserver >> >> >> so >> >> >> we shouldn't check for the other. >> >> > >> >> > Let me turn it around. We're checking for this one so we should also >> >> > check >> >> > for start_devserver. Using which grants wider path independence. >> >> >> >> I'm not sure you should check for it because it'll die anyway saying >> >> path can't be found. I'm not sure adding a Die with a re-wording will >> >> help much. The code should already be set -e >> > >> > OK. That makes sense, I'll re-hardcode it to /usr/bin/. > >
OK. SGTM. I'll change it. On Tue, Mar 1, 2011 at 9:14 PM, Chris Sosa <sosa@chromium.org> wrote: > On Tue, Mar 1, 2011 at 12:12 PM, Zdenek Behan <zbehan@chromium.org> wrote: > > > > > > On Tue, Mar 1, 2011 at 9:10 PM, Chris Sosa <sosa@chromium.org> wrote: > >> > >> On Tue, Mar 1, 2011 at 12:09 PM, Zdenek Behan <zbehan@chromium.org> > wrote: > >> > > >> > > >> > On Tue, Mar 1, 2011 at 8:57 PM, Chris Sosa <sosa@chromium.org> wrote: > >> >> > >> >> On Tue, Mar 1, 2011 at 10:38 AM, Zdenek Behan <zbehan@chromium.org> > >> >> wrote: > >> >> > I'm actually planning to get rid of platform/dev entirely (though > >> >> > this > >> >> > CL is > >> >> > not yet final for that). > >> >> > >> >> I was thinking that this CL pretty much eliminates the use of the use > >> >> of platform/dev/devserver code path except for use of > start_devserver. > >> >> If you remove the reference in that script, you can send out an > email > >> >> to chromiumos-dev to notify devs that changes to the devserver > >> >> requires working on it. Otherwise, it depends if you use > >> >> start_devsever or not. > >> > > >> > I believe that start_devserver already is eliminated. It should be > used > >> > exclusively from chroot by now. In fact the things that now explicitly > >> > depend on platform/dev/ don't have much/anything to do with devserver. > >> > Namely gmergefs and factory scripts (which are up for surgery next). I > >> > just > >> > didn't delete it from src/scripts/ yet, but I scrubbed every use of it > i > >> > found to call the chroot installation instead. > >> > >> start_devserver for many dev workflows is a first-level called script. > >> That's the issue ... it's not other calling scripts but devs invoking > >> it directly. > > > > So you're saying i shouldn't delete it, and instead make the scripts/ > > start_devserver call the chroot start_devserver? > > Well if you're not ready to delete in this CL, then yes, it seems to > make sense for it to invoke the chroot devserver so that all uses of > the devserver in the chroot go to the right place. > > >> > >> >> > >> >> > > >> >> > On Tue, Mar 1, 2011 at 6:40 PM, <sosa@chromium.org> wrote: > >> >> >> > >> >> >> You should modify start_devserver in crosutils to call > >> >> >> start_devserver > >> >> >> in > >> >> >> chroot > >> >> >> if not --archive_dir and after restart_in_chroot_if_needed ... > this > >> >> >> way > >> >> >> the > >> >> >> src/platform/dev call will not be used except for outside the > chroot > >> >> >> usage. > >> >> >> > >> >> >> > >> >> >> http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh > >> >> >> File image_to_live.sh (right): > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > http://codereview.chromium.org/6576023/diff/7001/image_to_live.sh#newcode225 > >> >> >> image_to_live.sh:225: if ! local > >> >> >> stateful_update_script="$(./enter_chroot.sh \ > >> >> >> Why is the which check necessary? We don't check for > >> >> >> start_devserver > >> >> >> so > >> >> >> we shouldn't check for the other. > >> >> > > >> >> > Let me turn it around. We're checking for this one so we should > also > >> >> > check > >> >> > for start_devserver. Using which grants wider path independence. > >> >> > >> >> I'm not sure you should check for it because it'll die anyway saying > >> >> path can't be found. I'm not sure adding a Die with a re-wording > will > >> >> help much. The code should already be set -e > >> > > >> > OK. That makes sense, I'll re-hardcode it to /usr/bin/. > > > > >
Done. PTAL. On Tue, Mar 1, 2011 at 9:38 PM, <zbehan@chromium.org> wrote: > http://codereview.chromium.org/6576023/ >
http://codereview.chromium.org/6576023/diff/10006/start_devserver File start_devserver (left): http://codereview.chromium.org/6576023/diff/10006/start_devserver#oldcode33 start_devserver:33: fi Do exec here. http://codereview.chromium.org/6576023/diff/10006/start_devserver File start_devserver (right): http://codereview.chromium.org/6576023/diff/10006/start_devserver#newcode4 start_devserver:4: exec $(dirname ${0})/../../chroot/usr/bin/start_devserver You should keep the old code until after the if if restart fi. Otherwise external ppl without chroots will die.
Heh. Yes, I suppose I should fix start_devserver to be callable from outside the chroot instead and then go with the original, but I'll do that in a separate CL. On Tue, Mar 1, 2011 at 10:39 PM, <sosa@chromium.org> wrote: > > http://codereview.chromium.org/6576023/diff/10006/start_devserver > File start_devserver (left): > > http://codereview.chromium.org/6576023/diff/10006/start_devserver#oldcode33 > start_devserver:33: fi > Do exec here. > > http://codereview.chromium.org/6576023/diff/10006/start_devserver > File start_devserver (right): > > http://codereview.chromium.org/6576023/diff/10006/start_devserver#newcode4 > start_devserver:4: exec $(dirname > ${0})/../../chroot/usr/bin/start_devserver > You should keep the old code until after the if if restart fi. > Otherwise external ppl without chroots will die. > > > http://codereview.chromium.org/6576023/ >
http://codereview.chromium.org/6576023/diff/11004/start_devserver File start_devserver (right): http://codereview.chromium.org/6576023/diff/11004/start_devserver#newcode35 start_devserver:35: exec $(dirname ${0})/../../chroot/usr/bin/start_devserver "$@" Heh ... so I realized this won't actually work for ppl who use this outside the chroot still. It seems like you'd need to put this code in the archive_dir logic and do cd ${GCLIENT_ROOT}/src/platform/dev && python devserver.py "$@" o/w
Why not? I tried running it from outside the chroot like this and it seems to be doing the right thing. On Tue, Mar 1, 2011 at 10:49 PM, <sosa@chromium.org> wrote: > > http://codereview.chromium.org/6576023/diff/11004/start_devserver > File start_devserver (right): > > http://codereview.chromium.org/6576023/diff/11004/start_devserver#newcode35 > start_devserver:35: exec $(dirname > ${0})/../../chroot/usr/bin/start_devserver "$@" > Heh ... so I realized this won't actually work for ppl who use this > outside the chroot still. It seems like you'd need to put this code in > the archive_dir logic and do cd ${GCLIENT_ROOT}/src/platform/dev && > python devserver.py "$@" o/w > > > http://codereview.chromium.org/6576023/ >
+Nick I believe partners who run the devserver using the factory flow never have a chroot. On Tue, Mar 1, 2011 at 1:51 PM, Zdenek Behan <zbehan@chromium.org> wrote: > Why not? I tried running it from outside the chroot like this and it seems > to be doing the right thing. > > On Tue, Mar 1, 2011 at 10:49 PM, <sosa@chromium.org> wrote: >> >> http://codereview.chromium.org/6576023/diff/11004/start_devserver >> File start_devserver (right): >> >> >> http://codereview.chromium.org/6576023/diff/11004/start_devserver#newcode35 >> start_devserver:35: exec $(dirname >> ${0})/../../chroot/usr/bin/start_devserver "$@" >> Heh ... so I realized this won't actually work for ppl who use this >> outside the chroot still. It seems like you'd need to put this code in >> the archive_dir logic and do cd ${GCLIENT_ROOT}/src/platform/dev && >> python devserver.py "$@" o/w >> >> http://codereview.chromium.org/6576023/ > >
But maybe they don't use this script ... if not I'm ok with this. On Tue, Mar 1, 2011 at 1:52 PM, Chris Sosa <sosa@chromium.org> wrote: > +Nick > > I believe partners who run the devserver using the factory flow never > have a chroot. > > On Tue, Mar 1, 2011 at 1:51 PM, Zdenek Behan <zbehan@chromium.org> wrote: >> Why not? I tried running it from outside the chroot like this and it seems >> to be doing the right thing. >> >> On Tue, Mar 1, 2011 at 10:49 PM, <sosa@chromium.org> wrote: >>> >>> http://codereview.chromium.org/6576023/diff/11004/start_devserver >>> File start_devserver (right): >>> >>> >>> http://codereview.chromium.org/6576023/diff/11004/start_devserver#newcode35 >>> start_devserver:35: exec $(dirname >>> ${0})/../../chroot/usr/bin/start_devserver "$@" >>> Heh ... so I realized this won't actually work for ppl who use this >>> outside the chroot still. It seems like you'd need to put this code in >>> the archive_dir logic and do cd ${GCLIENT_ROOT}/src/platform/dev && >>> python devserver.py "$@" o/w >>> >>> http://codereview.chromium.org/6576023/ >> >> >
The only "documented" use of devserver in the factory workflow is inside serve_factory_packages.sh, which uses devserver.py, rather than the shell wrapper. In fact the workflow is a little complicated and is my next target after this CL, but this CL should not affect it. On Tue, Mar 1, 2011 at 10:53 PM, Chris Sosa <sosa@chromium.org> wrote: > But maybe they don't use this script ... if not I'm ok with this. > > On Tue, Mar 1, 2011 at 1:52 PM, Chris Sosa <sosa@chromium.org> wrote: > > +Nick > > > > I believe partners who run the devserver using the factory flow never > > have a chroot. > > > > On Tue, Mar 1, 2011 at 1:51 PM, Zdenek Behan <zbehan@chromium.org> > wrote: > >> Why not? I tried running it from outside the chroot like this and it > seems > >> to be doing the right thing. > >> > >> On Tue, Mar 1, 2011 at 10:49 PM, <sosa@chromium.org> wrote: > >>> > >>> http://codereview.chromium.org/6576023/diff/11004/start_devserver > >>> File start_devserver (right): > >>> > >>> > >>> > http://codereview.chromium.org/6576023/diff/11004/start_devserver#newcode35 > >>> start_devserver:35: exec $(dirname > >>> ${0})/../../chroot/usr/bin/start_devserver "$@" > >>> Heh ... so I realized this won't actually work for ppl who use this > >>> outside the chroot still. It seems like you'd need to put this code in > >>> the archive_dir logic and do cd ${GCLIENT_ROOT}/src/platform/dev && > >>> python devserver.py "$@" o/w > >>> > >>> http://codereview.chromium.org/6576023/ > >> > >> > > >
sg, LGTM On Tue, Mar 1, 2011 at 1:58 PM, Zdenek Behan <zbehan@chromium.org> wrote: > The only "documented" use of devserver in the factory workflow is inside > serve_factory_packages.sh, which uses devserver.py, rather than the shell > wrapper. In fact the workflow is a little complicated and is my next target > after this CL, but this CL should not affect it. > > On Tue, Mar 1, 2011 at 10:53 PM, Chris Sosa <sosa@chromium.org> wrote: >> >> But maybe they don't use this script ... if not I'm ok with this. >> >> On Tue, Mar 1, 2011 at 1:52 PM, Chris Sosa <sosa@chromium.org> wrote: >> > +Nick >> > >> > I believe partners who run the devserver using the factory flow never >> > have a chroot. >> > >> > On Tue, Mar 1, 2011 at 1:51 PM, Zdenek Behan <zbehan@chromium.org> >> > wrote: >> >> Why not? I tried running it from outside the chroot like this and it >> >> seems >> >> to be doing the right thing. >> >> >> >> On Tue, Mar 1, 2011 at 10:49 PM, <sosa@chromium.org> wrote: >> >>> >> >>> http://codereview.chromium.org/6576023/diff/11004/start_devserver >> >>> File start_devserver (right): >> >>> >> >>> >> >>> >> >>> http://codereview.chromium.org/6576023/diff/11004/start_devserver#newcode35 >> >>> start_devserver:35: exec $(dirname >> >>> ${0})/../../chroot/usr/bin/start_devserver "$@" >> >>> Heh ... so I realized this won't actually work for ppl who use this >> >>> outside the chroot still. It seems like you'd need to put this code >> >>> in >> >>> the archive_dir logic and do cd ${GCLIENT_ROOT}/src/platform/dev && >> >>> python devserver.py "$@" o/w >> >>> >> >>> http://codereview.chromium.org/6576023/ >> >> >> >> >> > > > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
