Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6677163:
build_image: provide USE flags when installing chromeos-dev (Closed)
|
Created:
9 years, 8 months ago by zbehan Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, jimhebert, sosa Visibility:
Public. |
Descriptionbuild_image: provide USE flags when installing chromeos-dev
This fixes an issue where passing USE= to build_packages would always
cause build_image to fail.
BUG=none
TEST=create a USE-inconsistent package, run build_image, observe it working
Change-Id: Id9eb3891d9c292423c837a0d097a33155f03b794
R=davidjames@chromium.org,kliegs@chromium.org
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=b3cf233
Patch Set 1 #
Total comments: 5
Patch Set 2 : added -E #Messages
Total messages: 14 (0 generated)
Nits http://codereview.chromium.org/6677163/diff/1/build_image File build_image (right): http://codereview.chromium.org/6677163/diff/1/build_image#newcode421 build_image:421: sudo USE="${USE} ${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" \ Thanks, can you also add the -E argument to sudo while you're at it? http://codereview.chromium.org/6677163/diff/1/build_image#newcode509 build_image:509: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" ${EMERGE_BOARD_CMD} \ This should be updated as well to fix the --preserve case. http://codereview.chromium.org/6677163/diff/1/build_image#newcode514 build_image:514: sudo INSTALL_MASK="${INSTALL_MASK}" ${EMERGE_BOARD_CMD} \ Same here. http://codereview.chromium.org/6677163/diff/1/build_image#newcode641 build_image:641: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" ${EMERGE_BOARD_CMD} \ Strictly speaking this doesn't need USE flags but might as well set it here. And again, let's use sudo -E
Meta thought (probably suited for a later CL/cleantup) - it looks like the same sudo ... bit is done many times in the file. it seems moving it to a function would make everything more consistent especially for later changes. http://codereview.chromium.org/6677163/diff/1/build_image File build_image (right): http://codereview.chromium.org/6677163/diff/1/build_image#newcode423 build_image:423: --usepkgonly -uDNv chromeos-dev ${EMERGE_JOBS} I thought you'd mentioned on IRC the need to remove the 'N'?
On Tue, Apr 5, 2011 at 10:14 PM, <kliegs@chromium.org> wrote: > Meta thought (probably suited for a later CL/cleantup) - it looks like the > same > sudo ... bit is done many times in the file. it seems moving it to a > function > would make everything more consistent especially for later changes. Yeah, that's a good idea, but while you're at it, there's a hundred things about that script that could use refactoring. I'm not sure I want to get started on that endless chain. :) > http://codereview.chromium.org/6677163/diff/1/build_image > File build_image (right): > > http://codereview.chromium.org/6677163/diff/1/build_image#newcode423 > build_image:423: --usepkgonly -uDNv chromeos-dev ${EMERGE_JOBS} > I thought you'd mentioned on IRC the need to remove the 'N'? I didn't. Maybe davidjames did, but i think -N kinda makes sense here.
On Tue, Apr 5, 2011 at 10:09 PM, <davidjames@chromium.org> wrote: > Nits > > > http://codereview.chromium.org/6677163/diff/1/build_image > File build_image (right): > > http://codereview.chromium.org/6677163/diff/1/build_image#newcode421 > build_image:421: sudo USE="${USE} ${EXTRA_USE}" > INSTALL_MASK="${INSTALL_MASK}" \ > Thanks, can you also add the -E argument to sudo while you're at it? > OK. > > http://codereview.chromium.org/6677163/diff/1/build_image#newcode509 > build_image:509: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" > ${EMERGE_BOARD_CMD} \ > This should be updated as well to fix the --preserve case. > Updated how? By adding $USE into the current USE= statement? I generally didn't want to touch unrelated use cases to what I was fixing. > http://codereview.chromium.org/6677163/diff/1/build_image#newcode514 > build_image:514: sudo INSTALL_MASK="${INSTALL_MASK}" ${EMERGE_BOARD_CMD} > \ > Same here. > > http://codereview.chromium.org/6677163/diff/1/build_image#newcode641 > build_image:641: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" > ${EMERGE_BOARD_CMD} \ > Strictly speaking this doesn't need USE flags but might as well set it > here. > I thought about this for a while, and I believe there's an actual practical use for this. For example, USE flag enabled could pull in another package via DEPEND. If that USE flag is only enabled for the first command, the dependency might get removed immediately. Then again, maybe that's just a speculation and portage is smart enough to look at the flags of the installed packages, not would-be-installed. Still, doesn't hurt to play it safe. > And again, let's use sudo -E > > > http://codereview.chromium.org/6677163/ >
I don't think this CL works for me then to fix it - I'll test again. But its only the chromeos-dev package that was breaking, others were working fine. Unless this is fixing a different use case. On Tue, Apr 5, 2011 at 4:20 PM, Zdenek Behan <zbehan@chromium.org> wrote: > > > On Tue, Apr 5, 2011 at 10:09 PM, <davidjames@chromium.org> wrote: > >> Nits >> >> >> http://codereview.chromium.org/6677163/diff/1/build_image >> File build_image (right): >> >> http://codereview.chromium.org/6677163/diff/1/build_image#newcode421 >> build_image:421: sudo USE="${USE} ${EXTRA_USE}" >> INSTALL_MASK="${INSTALL_MASK}" \ >> Thanks, can you also add the -E argument to sudo while you're at it? >> > > OK. > > >> >> http://codereview.chromium.org/6677163/diff/1/build_image#newcode509 >> build_image:509: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" >> ${EMERGE_BOARD_CMD} \ >> This should be updated as well to fix the --preserve case. >> > > Updated how? By adding $USE into the current USE= statement? I generally > didn't want to touch unrelated use cases to what I was fixing. > > >> http://codereview.chromium.org/6677163/diff/1/build_image#newcode514 >> build_image:514: sudo INSTALL_MASK="${INSTALL_MASK}" ${EMERGE_BOARD_CMD} >> \ >> Same here. >> >> http://codereview.chromium.org/6677163/diff/1/build_image#newcode641 >> build_image:641: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" >> ${EMERGE_BOARD_CMD} \ >> Strictly speaking this doesn't need USE flags but might as well set it >> here. >> > > I thought about this for a while, and I believe there's an actual practical > use for this. For example, USE flag enabled could pull in another package > via DEPEND. If that USE flag is only enabled for the first command, the > dependency might get removed immediately. Then again, maybe that's just a > speculation and portage is smart enough to look at the flags of the > installed packages, not would-be-installed. Still, doesn't hurt to play it > safe. > > >> And again, let's use sudo -E >> >> >> http://codereview.chromium.org/6677163/ >> > >
Well, the original feature where packages are pulled in regardless of system USE settings if --usepkgonly was specified is gone, or so I understood what davidjames explained. So you have to pass USE= to both build_packages and build_image, or simply export it in your shell. The point of this CL is, that passing USE= did nothing for build_image, and that is fixed now. On Tue, Apr 5, 2011 at 10:23 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > I don't think this CL works for me then to fix it - I'll test again. > > But its only the chromeos-dev package that was breaking, others were > working fine. Unless this is fixing a different use case. > > > On Tue, Apr 5, 2011 at 4:20 PM, Zdenek Behan <zbehan@chromium.org> wrote: > >> >> >> On Tue, Apr 5, 2011 at 10:09 PM, <davidjames@chromium.org> wrote: >> >>> Nits >>> >>> >>> http://codereview.chromium.org/6677163/diff/1/build_image >>> File build_image (right): >>> >>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode421 >>> build_image:421: sudo USE="${USE} ${EXTRA_USE}" >>> INSTALL_MASK="${INSTALL_MASK}" \ >>> Thanks, can you also add the -E argument to sudo while you're at it? >>> >> >> OK. >> >> >>> >>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode509 >>> build_image:509: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" >>> ${EMERGE_BOARD_CMD} \ >>> This should be updated as well to fix the --preserve case. >>> >> >> Updated how? By adding $USE into the current USE= statement? I generally >> didn't want to touch unrelated use cases to what I was fixing. >> >> >>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode514 >>> build_image:514: sudo INSTALL_MASK="${INSTALL_MASK}" ${EMERGE_BOARD_CMD} >>> \ >>> Same here. >>> >>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode641 >>> build_image:641: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" >>> ${EMERGE_BOARD_CMD} \ >>> Strictly speaking this doesn't need USE flags but might as well set it >>> here. >>> >> >> I thought about this for a while, and I believe there's an actual >> practical use for this. For example, USE flag enabled could pull in another >> package via DEPEND. If that USE flag is only enabled for the first command, >> the dependency might get removed immediately. Then again, maybe that's just >> a speculation and portage is smart enough to look at the flags of the >> installed packages, not would-be-installed. Still, doesn't hurt to play it >> safe. >> >> >>> And again, let's use sudo -E >>> >>> >>> http://codereview.chromium.org/6677163/ >>> >> >> >
OK, LGTM, but can you file a bug to fix the review concerns you didn't fix in this CL?
Are you sure about that? I just did a test where I did: USE=touchui ./build_packages emerge-board chromeos-dev (to clear the USE flag on chromeos-dev) ./build_image And build_image worked, installing the packages that had that USE flag set in build_packages but not in build_image It was only if chromeos-dev had that flag that it would fail (and possibly packages that chromeos-dev is dependent on). On Tue, Apr 5, 2011 at 4:25 PM, Zdenek Behan <zbehan@chromium.org> wrote: > Well, the original feature where packages are pulled in regardless of > system USE settings if --usepkgonly was specified is gone, or so I > understood what davidjames explained. So you have to pass USE= to both > build_packages and build_image, or simply export it in your shell. > > The point of this CL is, that passing USE= did nothing for build_image, and > that is fixed now. > > > On Tue, Apr 5, 2011 at 10:23 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > >> I don't think this CL works for me then to fix it - I'll test again. >> >> But its only the chromeos-dev package that was breaking, others were >> working fine. Unless this is fixing a different use case. >> >> >> On Tue, Apr 5, 2011 at 4:20 PM, Zdenek Behan <zbehan@chromium.org> wrote: >> >>> >>> >>> On Tue, Apr 5, 2011 at 10:09 PM, <davidjames@chromium.org> wrote: >>> >>>> Nits >>>> >>>> >>>> http://codereview.chromium.org/6677163/diff/1/build_image >>>> File build_image (right): >>>> >>>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode421 >>>> build_image:421: sudo USE="${USE} ${EXTRA_USE}" >>>> INSTALL_MASK="${INSTALL_MASK}" \ >>>> Thanks, can you also add the -E argument to sudo while you're at it? >>>> >>> >>> OK. >>> >>> >>>> >>>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode509 >>>> build_image:509: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" >>>> ${EMERGE_BOARD_CMD} \ >>>> This should be updated as well to fix the --preserve case. >>>> >>> >>> Updated how? By adding $USE into the current USE= statement? I generally >>> didn't want to touch unrelated use cases to what I was fixing. >>> >>> >>>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode514 >>>> build_image:514: sudo INSTALL_MASK="${INSTALL_MASK}" ${EMERGE_BOARD_CMD} >>>> \ >>>> Same here. >>>> >>>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode641 >>>> build_image:641: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" >>>> ${EMERGE_BOARD_CMD} \ >>>> Strictly speaking this doesn't need USE flags but might as well set it >>>> here. >>>> >>> >>> I thought about this for a while, and I believe there's an actual >>> practical use for this. For example, USE flag enabled could pull in another >>> package via DEPEND. If that USE flag is only enabled for the first command, >>> the dependency might get removed immediately. Then again, maybe that's just >>> a speculation and portage is smart enough to look at the flags of the >>> installed packages, not would-be-installed. Still, doesn't hurt to play it >>> safe. >>> >>> >>>> And again, let's use sudo -E >>>> >>>> >>>> http://codereview.chromium.org/6677163/ >>>> >>> >>> >> >
Maybe a leftover from your experiment where you put touchui into make.conf? On Tue, Apr 5, 2011 at 10:35 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: > Are you sure about that? I just did a test where I did: > > USE=touchui ./build_packages > emerge-board chromeos-dev (to clear the USE flag on chromeos-dev) > ./build_image > And build_image worked, installing the packages that had that USE flag set > in build_packages but not in build_image > > It was only if chromeos-dev had that flag that it would fail (and possibly > packages that chromeos-dev is dependent on). > > On Tue, Apr 5, 2011 at 4:25 PM, Zdenek Behan <zbehan@chromium.org> wrote: > >> Well, the original feature where packages are pulled in regardless of >> system USE settings if --usepkgonly was specified is gone, or so I >> understood what davidjames explained. So you have to pass USE= to both >> build_packages and build_image, or simply export it in your shell. >> >> The point of this CL is, that passing USE= did nothing for build_image, >> and that is fixed now. >> >> >> On Tue, Apr 5, 2011 at 10:23 PM, Jonathan Kliegman <kliegs@chromium.org>wrote: >> >>> I don't think this CL works for me then to fix it - I'll test again. >>> >>> But its only the chromeos-dev package that was breaking, others were >>> working fine. Unless this is fixing a different use case. >>> >>> >>> On Tue, Apr 5, 2011 at 4:20 PM, Zdenek Behan <zbehan@chromium.org>wrote: >>> >>>> >>>> >>>> On Tue, Apr 5, 2011 at 10:09 PM, <davidjames@chromium.org> wrote: >>>> >>>>> Nits >>>>> >>>>> >>>>> http://codereview.chromium.org/6677163/diff/1/build_image >>>>> File build_image (right): >>>>> >>>>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode421 >>>>> build_image:421: sudo USE="${USE} ${EXTRA_USE}" >>>>> INSTALL_MASK="${INSTALL_MASK}" \ >>>>> Thanks, can you also add the -E argument to sudo while you're at it? >>>>> >>>> >>>> OK. >>>> >>>> >>>>> >>>>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode509 >>>>> build_image:509: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" >>>>> ${EMERGE_BOARD_CMD} \ >>>>> This should be updated as well to fix the --preserve case. >>>>> >>>> >>>> Updated how? By adding $USE into the current USE= statement? I generally >>>> didn't want to touch unrelated use cases to what I was fixing. >>>> >>>> >>>>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode514 >>>>> build_image:514: sudo INSTALL_MASK="${INSTALL_MASK}" >>>>> ${EMERGE_BOARD_CMD} >>>>> \ >>>>> Same here. >>>>> >>>>> http://codereview.chromium.org/6677163/diff/1/build_image#newcode641 >>>>> build_image:641: sudo USE="${EXTRA_USE}" INSTALL_MASK="${INSTALL_MASK}" >>>>> ${EMERGE_BOARD_CMD} \ >>>>> Strictly speaking this doesn't need USE flags but might as well set it >>>>> here. >>>>> >>>> >>>> I thought about this for a while, and I believe there's an actual >>>> practical use for this. For example, USE flag enabled could pull in another >>>> package via DEPEND. If that USE flag is only enabled for the first command, >>>> the dependency might get removed immediately. Then again, maybe that's just >>>> a speculation and portage is smart enough to look at the flags of the >>>> installed packages, not would-be-installed. Still, doesn't hurt to play it >>>> safe. >>>> >>>> >>>>> And again, let's use sudo -E >>>>> >>>>> >>>>> http://codereview.chromium.org/6677163/ >>>>> >>>> >>>> >>> >> >
http://code.google.com/p/chromium-os/issues/detail?id=13852 I don't think that is a priority, plus I don't want to have my CL reverted because I touched that and broke someone's obscure workflow in the depths of china. On Tue, Apr 5, 2011 at 10:31 PM, <davidjames@chromium.org> wrote: > OK, LGTM, but can you file a bug to fix the review concerns you didn't fix > in > this CL? > > > http://codereview.chromium.org/6677163/ >
I agree that doing the refactoring now isn't appropriate. Was more mentioning it as "something to keep in mind for later". As for my test case, I just verified again: USE=touchui build_packages build_image #failed emerge-board chromeos-dev # installed now without touchui build_image #worked On Tue, Apr 5, 2011 at 4:38 PM, Zdenek Behan <zbehan@chromium.org> wrote: > http://code.google.com/p/chromium-os/issues/detail?id=13852 > > I don't think that is a priority, plus I don't want to have my CL reverted > because I touched that and broke someone's obscure workflow in the depths of > china. > > > On Tue, Apr 5, 2011 at 10:31 PM, <davidjames@chromium.org> wrote: > >> OK, LGTM, but can you file a bug to fix the review concerns you didn't fix >> in >> this CL? >> >> >> http://codereview.chromium.org/6677163/ >> > >
LGTM While my particular case failing isn't fixed by this, the CL itself fixes other bugs and its not worth holding up. On 2011/04/05 20:56:21, kliegs wrote: > I agree that doing the refactoring now isn't appropriate. Was more > mentioning it as "something to keep in mind for later". > > As for my test case, I just verified again: > USE=touchui build_packages > build_image #failed > emerge-board chromeos-dev # installed now without touchui > build_image #worked > > > > > On Tue, Apr 5, 2011 at 4:38 PM, Zdenek Behan <mailto:zbehan@chromium.org> wrote: > > > http://code.google.com/p/chromium-os/issues/detail?id=13852 > > > > I don't think that is a priority, plus I don't want to have my CL reverted > > because I touched that and broke someone's obscure workflow in the depths of > > china. > > > > > > On Tue, Apr 5, 2011 at 10:31 PM, <mailto:davidjames@chromium.org> wrote: > > > >> OK, LGTM, but can you file a bug to fix the review concerns you didn't fix > >> in > >> this CL? > >> > >> > >> http://codereview.chromium.org/6677163/ > >> > > > > |
