Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
898613002:
[NaCl SDK] Use separate gyp output folders for x64 and ia32 (Closed)
Description[NaCl SDK] Use separate gyp output folders for x64 and ia32
Previously we were re-using 'gypbuild' for both 64 and 32.
This change increases the complexity of the build but
is more correct and robust and allows us to avoid cleaning
the gyp output folders between builds.
Committed: https://crrev.com/ed1472c966138ca149e5754ca1e5cc05d06e2349
Cr-Commit-Position: refs/heads/master@{#314898}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #
Total comments: 1
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 4
Patch Set 9 : #Messages
Total messages: 20 (3 generated)
sbc@chromium.org changed reviewers: + binji@chromium.org
What about this Windows failure? Is it unrelated? s:\src\native_client\src\shared\platform\win\nacl_find_addrsp.c(59) : warning C4293: '<<' : shift count negative or too big, undefined behavior s https://codereview.chromium.org/898613002/diff/40001/native_client_sdk/src/bu... File native_client_sdk/src/build_tools/build_sdk.py (left): https://codereview.chromium.org/898613002/diff/40001/native_client_sdk/src/bu... native_client_sdk/src/build_tools/build_sdk.py:566: buildbot_common.RemoveDir(os.path.join(OUT_DIR, GYPBUILD_DIR)) I think we still want to do this, at least on the build bots. Perhaps there should be a flag.
https://codereview.chromium.org/898613002/diff/40001/native_client_sdk/src/bu... File native_client_sdk/src/build_tools/build_sdk.py (left): https://codereview.chromium.org/898613002/diff/40001/native_client_sdk/src/bu... native_client_sdk/src/build_tools/build_sdk.py:566: buildbot_common.RemoveDir(os.path.join(OUT_DIR, GYPBUILD_DIR)) On 2015/02/04 19:56:03, binji wrote: > I think we still want to do this, at least on the build bots. Perhaps there > should be a flag. Why do you think we should do this? If gyp output directories need to be clobbers we should just clobber the whole 'out' folder, no?
On 2015/02/04 20:40:17, Sam Clegg wrote: > https://codereview.chromium.org/898613002/diff/40001/native_client_sdk/src/bu... > File native_client_sdk/src/build_tools/build_sdk.py (left): > > https://codereview.chromium.org/898613002/diff/40001/native_client_sdk/src/bu... > native_client_sdk/src/build_tools/build_sdk.py:566: > buildbot_common.RemoveDir(os.path.join(OUT_DIR, GYPBUILD_DIR)) > On 2015/02/04 19:56:03, binji wrote: > > I think we still want to do this, at least on the build bots. Perhaps there > > should be a flag. > > Why do you think we should do this? > > If gyp output directories need to be clobbers we should just clobber the whole > 'out' folder, no? I don't think this is what happens for clobber builds, and we definitely want the SDK buildbots to be clobber builds. I added this (and you reviewed it) because without clobbers we were hiding build failures. I have no problem with keeping incremental builds, but that should be something that happens locally, not on the bots.
On 2015/02/04 21:23:58, binji wrote: > On 2015/02/04 20:40:17, Sam Clegg wrote: > > > https://codereview.chromium.org/898613002/diff/40001/native_client_sdk/src/bu... > > File native_client_sdk/src/build_tools/build_sdk.py (left): > > > > > https://codereview.chromium.org/898613002/diff/40001/native_client_sdk/src/bu... > > native_client_sdk/src/build_tools/build_sdk.py:566: > > buildbot_common.RemoveDir(os.path.join(OUT_DIR, GYPBUILD_DIR)) > > On 2015/02/04 19:56:03, binji wrote: > > > I think we still want to do this, at least on the build bots. Perhaps there > > > should be a flag. > > > > Why do you think we should do this? > > > > If gyp output directories need to be clobbers we should just clobber the whole > > 'out' folder, no? > > I don't think this is what happens for clobber builds, and we definitely want > the SDK buildbots to be clobber builds. Hmm.. presumably we want them to be clobber builds for the whole of chrome too? In which case in the long run shouldn't really on something further up the stack to remove the entire 'out' tree (i.e. clobber everything)? > I added this (and you reviewed it) because without clobbers we were hiding build > failures. OK, I added this back. I was assuming this was because were use using the same gyp output_dir for multiple invocations (which this change fixes). > > I have no problem with keeping incremental builds, but that should be something > that happens locally, not on the bots. Done.
On 2015/02/04 22:23:55, Sam Clegg wrote: > On 2015/02/04 21:23:58, binji wrote: > > On 2015/02/04 20:40:17, Sam Clegg wrote: > > > > > > https://codereview.chromium.org/898613002/diff/40001/native_client_sdk/src/bu... > > > File native_client_sdk/src/build_tools/build_sdk.py (left): > > > > > > > > > https://codereview.chromium.org/898613002/diff/40001/native_client_sdk/src/bu... > > > native_client_sdk/src/build_tools/build_sdk.py:566: > > > buildbot_common.RemoveDir(os.path.join(OUT_DIR, GYPBUILD_DIR)) > > > On 2015/02/04 19:56:03, binji wrote: > > > > I think we still want to do this, at least on the build bots. Perhaps > there > > > > should be a flag. > > > > > > Why do you think we should do this? > > > > > > If gyp output directories need to be clobbers we should just clobber the > whole > > > 'out' folder, no? > > > > I don't think this is what happens for clobber builds, and we definitely want > > the SDK buildbots to be clobber builds. > > Hmm.. presumably we want them to be clobber builds for the whole of chrome too? > In which case in the long run shouldn't really on something further up the stack > to remove the entire 'out' tree (i.e. clobber everything)? Maybe so, but until we have that I'd like to prevent these regressions. You could add a TODO, I suppose. > > > I added this (and you reviewed it) because without clobbers we were hiding > build > > failures. > > OK, I added this back. > > I was assuming this was because were use using the same gyp output_dir for > multiple invocations (which this change fixes). > > > > > I have no problem with keeping incremental builds, but that should be > something > > that happens locally, not on the bots. > > Done. lgtm
https://codereview.chromium.org/898613002/diff/80001/native_client_sdk/src/bu... File native_client_sdk/src/build_tools/build_sdk.py (right): https://codereview.chromium.org/898613002/diff/80001/native_client_sdk/src/bu... native_client_sdk/src/build_tools/build_sdk.py:939: help="Don't clean gybbuild directories") I believe you still have to set this to default=True Or has that changed with argparse? Just checked, they fixed this. :-)
On 2015/02/04 22:45:35, binji wrote: > https://codereview.chromium.org/898613002/diff/80001/native_client_sdk/src/bu... > File native_client_sdk/src/build_tools/build_sdk.py (right): > > https://codereview.chromium.org/898613002/diff/80001/native_client_sdk/src/bu... > native_client_sdk/src/build_tools/build_sdk.py:939: help="Don't clean gybbuild > directories") > I believe you still have to set this to default=True > > Or has that changed with argparse? Just checked, they fixed this. :-) Yes, I verified that the default is True if you set action="store_false".
On 2015/02/04 22:45:35, binji wrote: > https://codereview.chromium.org/898613002/diff/80001/native_client_sdk/src/bu... > File native_client_sdk/src/build_tools/build_sdk.py (right): > > https://codereview.chromium.org/898613002/diff/80001/native_client_sdk/src/bu... > native_client_sdk/src/build_tools/build_sdk.py:939: help="Don't clean gybbuild > directories") > I believe you still have to set this to default=True > > Or has that changed with argparse? Just checked, they fixed this. :-) Also, please confirm that the win trybot failure is not related to this CL.
On 2015/02/04 22:47:55, binji wrote: > On 2015/02/04 22:45:35, binji wrote: > > > https://codereview.chromium.org/898613002/diff/80001/native_client_sdk/src/bu... > > File native_client_sdk/src/build_tools/build_sdk.py (right): > > > > > https://codereview.chromium.org/898613002/diff/80001/native_client_sdk/src/bu... > > native_client_sdk/src/build_tools/build_sdk.py:939: help="Don't clean gybbuild > > directories") > > I believe you still have to set this to default=True > > > > Or has that changed with argparse? Just checked, they fixed this. :-) > > Also, please confirm that the win trybot failure is not related to this CL. It was related. And I fixed that in the last upload. Current mac failure look related to bbudge@ latest change..
New patchsets have been uploaded after l-g-t-m from binji@chromium.org
Simplified this patch a little and fixed windows issues. One side effect of this change is that we now ship ncval as a 64bit binary on windows like we already did on other platforms. PTAL
slgtm https://codereview.chromium.org/898613002/diff/140001/native_client_sdk/src/b... File native_client_sdk/src/build_tools/build_sdk.py (right): https://codereview.chromium.org/898613002/diff/140001/native_client_sdk/src/b... native_client_sdk/src/build_tools/build_sdk.py:541: # We can't use windows path sepertors in GYP_GENERATOR_FLAGS since separators https://codereview.chromium.org/898613002/diff/140001/native_client_sdk/src/b... native_client_sdk/src/build_tools/build_sdk.py:935: help="Don't clean gybbuild directories") gypbuild
https://codereview.chromium.org/898613002/diff/140001/native_client_sdk/src/b... File native_client_sdk/src/build_tools/build_sdk.py (right): https://codereview.chromium.org/898613002/diff/140001/native_client_sdk/src/b... native_client_sdk/src/build_tools/build_sdk.py:541: # We can't use windows path sepertors in GYP_GENERATOR_FLAGS since On 2015/02/05 21:14:01, binji wrote: > separators Done. https://codereview.chromium.org/898613002/diff/140001/native_client_sdk/src/b... native_client_sdk/src/build_tools/build_sdk.py:935: help="Don't clean gybbuild directories") On 2015/02/05 21:14:01, binji wrote: > gypbuild Done.
The CQ bit was checked by sbc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/898613002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/ed1472c966138ca149e5754ca1e5cc05d06e2349 Cr-Commit-Position: refs/heads/master@{#314898} |
