|
|
DescriptionChanges exception for gyp files to only gyp files in build/
I changed analyzer such that if a gyp file (or an included gypi file)
changes all targets in the gyp file are marked as changed. This means
I should no longer have to special case gyp files. I'm leaving the
exception in place for any gyp files in build as any change to those
really needs to recompile the world.
BUG=109173
TEST=none
R=thakis@chromium.org
Committed to pending queue: https://chromium.googlesource.com/chromium/src/+/5110e8b
Committed: https://chromium.googlesource.com/chromium/src/+/e53e29fc161f96871160f6db39919752a0376333
Committed: https://chromium.googlesource.com/chromium/src/+/5c361bce306a985bcb65cd9db6060ccbe139f66d
Committed: https://crrev.com/3f631981fd3ecd25ecda9497c19e63f25a557313
Cr-Commit-Position: refs/heads/master@{#293983}
Patch Set 1 #
Messages
Total messages: 23 (0 generated)
"""I changed analyzer such that if a gyp file (or an included gypi file) changes all targets in the gyp file are marked as changed.""" Is that enough? Say a gyp target does 'all_dependent_targets': { 'defines': [ 'FOO' ], } and then the define changes. Now all downstream dependencies need to be built too. (On a related note: the build system does most of the "build only what's needed" for you if you touch all existing output files before applying the patch. The only thing this won't catch is if the build directory doesn't have some .o files for newly-added .cc files not needed by the current patch. I don't know how many try runs this would affect, but trying to use the build system instead of reimplementing the logic might have fewer bugs long term.)
On Thu, Aug 28, 2014 at 7:48 AM, <thakis@chromium.org> wrote: > """I changed analyzer such that if a gyp file (or an included gypi file) > changes all targets in the gyp file are marked as changed.""" > > Is that enough? Say a gyp target does > > 'all_dependent_targets': { > 'defines': [ 'FOO' ], > } > > and then the define changes. Now all downstream dependencies need to be > built > too. Do you mean all_dependent_settings? If so, then it should since building a target implicitly builds dependents. > (On a related note: the build system does most of the "build only what's > needed" > for you if you touch all existing output files before applying the patch. That's a good idea. I need to think about this. -Scott > The > only thing this won't catch is if the build directory doesn't have some .o > files > for newly-added .cc files not needed by the current patch. I don't know how > many > try runs this would affect, but trying to use the build system instead of > reimplementing the logic might have fewer bugs long term.) To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Aug 28, 2014 at 8:32 AM, Scott Violet <sky@chromium.org> wrote: > On Thu, Aug 28, 2014 at 7:48 AM, <thakis@chromium.org> wrote: > > """I changed analyzer such that if a gyp file (or an included gypi file) > > changes all targets in the gyp file are marked as changed.""" > > > > Is that enough? Say a gyp target does > > > > 'all_dependent_targets': { > > 'defines': [ 'FOO' ], > > } > > > > and then the define changes. Now all downstream dependencies need to be > > built > > too. > > Do you mean all_dependent_settings? Yes, sorry. > If so, then it should since > building a target implicitly builds dependents. > We might be talking past each other here, so sorry if the following is needlessly long. Consider this dependency chain: base <- common <- chrome If you give common an all_dependent_settings, then it pushes stuff in there to all downstream deps, in this example to 'chrome'. Say content has a public header that needs some define to be set, so it sets this define on all its downstream dependencies (here, chrome). If you build common, this implicitly builds all upstream dependencies (here, base). So if content.gyp is changed to change the define, then you must build target chrome (since its defines have changed), not just content (which would implicitly compile content and base). > > > (On a related note: the build system does most of the "build only what's > > needed" > > for you if you touch all existing output files before applying the patch. > > That's a good idea. I need to think about this. > > -Scott > > > The > > only thing this won't catch is if the build directory doesn't have some > .o > > files > > for newly-added .cc files not needed by the current patch. I don't know > how > > many > > try runs this would affect, but trying to use the build system instead of > > reimplementing the logic might have fewer bugs long term.) > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Aug 28, 2014 at 8:36 AM, Nico Weber <thakis@chromium.org> wrote: > On Thu, Aug 28, 2014 at 8:32 AM, Scott Violet <sky@chromium.org> wrote: >> >> On Thu, Aug 28, 2014 at 7:48 AM, <thakis@chromium.org> wrote: >> > """I changed analyzer such that if a gyp file (or an included gypi file) >> > changes all targets in the gyp file are marked as changed.""" >> > >> > Is that enough? Say a gyp target does >> > >> > 'all_dependent_targets': { >> > 'defines': [ 'FOO' ], >> > } >> > >> > and then the define changes. Now all downstream dependencies need to be >> > built >> > too. >> >> Do you mean all_dependent_settings? > > > Yes, sorry. > >> >> If so, then it should since >> building a target implicitly builds dependents. > > > We might be talking past each other here, so sorry if the following is > needlessly long. > > Consider this dependency chain: > > base <- common <- chrome > > If you give common an all_dependent_settings, then it pushes stuff in there > to all downstream deps, in this example to 'chrome'. Say content has a > public header that needs some define to be set, so it sets this define on > all its downstream dependencies (here, chrome). > > If you build common, this implicitly builds all upstream dependencies (here, > base). So if content.gyp is changed to change the define, then you must > build target chrome (since its defines have changed), not just content > (which would implicitly compile content and base). For the example you give 'chrome' would be output as a build_target as long as it is not type 'none', or if it's type 'none' but has actions/rules. The first part of analyzer determines the set of targets that reference the changed files. If a gyp/gypi file changes then all of the targets in the file are considered changed. One of the follow phases is to determine the set of build targets. The build targets are the targets found by going down in the dependency graph including all targets that are not type none or none with actions/rules. This is the phase that would pick up 'chrome' in your example. As a side node it's interesting you consider chrome downstream of common, I always view the graph as: chrome->common->base, in which case chrome is upstream. Anyway. -Scott > >> >> >> > (On a related note: the build system does most of the "build only what's >> > needed" >> > for you if you touch all existing output files before applying the >> > patch. >> >> That's a good idea. I need to think about this. >> >> -Scott >> >> > The >> > only thing this won't catch is if the build directory doesn't have some >> > .o >> > files >> > for newly-added .cc files not needed by the current patch. I don't know >> > how >> > many >> > try runs this would affect, but trying to use the build system instead >> > of >> > reimplementing the logic might have fewer bugs long term.) > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> For the example you give 'chrome' would be output as a build_target as > long as it is not type 'none', or if it's type 'none' but has > actions/rules. The first part of analyzer determines the set of > targets that reference the changed files. If a gyp/gypi file changes > then all of the targets in the file are considered changed. One of the > follow phases is to determine the set of build targets. The build > targets are the targets found by going down in the dependency graph > including all targets that are not type none or none with > actions/rules. This is the phase that would pick up 'chrome' in your > example. Ah, analyze handles this! Then lgtm, I thought you mean ninja would do that. > As a side node it's interesting you consider chrome downstream of > common, I always view the graph as: chrome->common->base, in which > case chrome is upstream. Anyway. :-) I think I use both orders. Your interpretation makes more sense when talking about builds, the other direction makes sense in the "upstream project / downstream project" sense I think. (As in "base in our upstream project, we must track changes in it. What base dumps in the river, we need to bathe in.")
Message was sent while issue was closed.
Committed patchset #1 (id:1) to pending queue manually as 5110e8b (presubmit successful).
Message was sent while issue was closed.
I'm reverting this. http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_... should have triggered a compile.
Message was sent while issue was closed.
A revert of this CL (patchset #1) has been created in https://codereview.chromium.org/512303002/ by sky@chromium.org. The reason for reverting is: I'm reverting this. http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_... should have triggered a compile..
I fixed the problem in the analyzer side and rolled deps. So, I'm going to reland this.
ok!
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as e53e29f (presubmit successful).
Message was sent while issue was closed.
On 2014/09/03 15:41:26, sky wrote: > Committed patchset #1 (id:1) manually as e53e29f (presubmit successful). *SIGH* Encountered another problem. See http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... . Reverting again.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/537683002/ by sky@chromium.org. The reason for reverting is: *SIGH* Encountered another problem. See http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... . Reverting again..
I'm trying this for a third time. Hopefully it sticks.
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 5c361bc (presubmit successful).
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/553933002/ by jochen@chromium.org. The reason for reverting is: gyp change this depended on was reverted.
Reopenning and trying for a 4th time. Yeesh!
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 3f63198 (presubmit successful).
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/623d2d8aae03f12cc9854b9f9c26afeb82da2ea4 Cr-Commit-Position: refs/heads/master@{#292392}
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/e53e29fc161f96871160f6db39919752a0376333 Cr-Commit-Position: refs/heads/master@{#293139}
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/5c361bce306a985bcb65cd9db6060ccbe139f66d Cr-Commit-Position: refs/heads/master@{#293565}
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/3f631981fd3ecd25ecda9497c19e63f25a557313 Cr-Commit-Position: refs/heads/master@{#293983} |