Chromium Code Reviews
[email protected] (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(278)

Issue 1430393002: Specify dependencies on generate_version_info target correctly (Closed)

Created:
5 years, 1 month ago by blundell
Modified:
5 years ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Specify dependencies on generate_version_info target correctly The generate_version_info target generates a header (version_info_values.h) and correctly specifies itself as a hard dependency. Currently, most targets that use this header don't depend directly on generate_version_info but rather on the version_info target. version_info depends on generate_version_info and does export_dependent_settings on it. However, version_info actually shouldn't export_dependent_settings on generate_version_info, because it includes version_info_values.h only in a cc file, not in a header. Perhaps for this reason, the current structure doesn't actually work: if you do a clobber build of one of the targets that depends on generate_version_info via version_info (e.g., //components/browser_sync/browser), compilation will fail due to not finding components/version_info/version_info_values.h. My speculation is that either gyp or ninja correctly determines that the hard dependency from (e.g.) //components/browser_sync/browser to //components/version_info isn't actually necessary, but isn't smart enough to add the missing dependency on generate_version_info. This CL changes all targets that include components/version_info/version_info_values.h to depend directly on generate_version_info and removes the export_dependent_settings that version_info previously had on generate_version_info. BUG=553647 Committed: https://crrev.com/f1cfb03ae9ebf5dc1e2dd8dc96f6976dc20209c7 Cr-Commit-Position: refs/heads/master@{#359867}

Patch Set 1 #

Patch Set 2 : Fixes #

Patch Set 3 : nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -8 lines) Patch
M chrome/browser/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/browser_sync/browser/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/version_info.gypi View 1 chunk +0 lines, -3 lines 0 comments Download
M components/version_info/BUILD.gn View 1 1 chunk +1 line, -5 lines 0 comments Download

Messages

Total messages: 25 (6 generated)
blundell
5 years, 1 month ago (2015-11-10 10:08:58 UTC) #2
droger
lgtm
5 years, 1 month ago (2015-11-10 10:42:36 UTC) #3
blundell
+thakis for //chrome
5 years, 1 month ago (2015-11-10 10:43:45 UTC) #5
blundell
On 2015/11/10 10:43:45, blundell wrote: > +thakis for //chrome (note that the red bots are ...
5 years, 1 month ago (2015-11-10 10:44:49 UTC) #6
Olivier
lgtm
5 years, 1 month ago (2015-11-10 10:59:26 UTC) #8
blundell
On 2015/11/10 10:44:49, blundell wrote: > On 2015/11/10 10:43:45, blundell wrote: > > +thakis for ...
5 years, 1 month ago (2015-11-10 11:05:01 UTC) #9
blundell
thakis: ping
5 years, 1 month ago (2015-11-13 08:21:51 UTC) #11
blundell
+jochen@ Could you look at //chrome? Thanks!
5 years, 1 month ago (2015-11-16 10:31:55 UTC) #13
jochen (gone - plz use gerrit)
lgtm
5 years, 1 month ago (2015-11-16 11:59:10 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1430393002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1430393002/40001
5 years, 1 month ago (2015-11-16 17:04:59 UTC) #16
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 1 month ago (2015-11-16 18:22:41 UTC) #17
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/f1cfb03ae9ebf5dc1e2dd8dc96f6976dc20209c7 Cr-Commit-Position: refs/heads/master@{#359867}
5 years, 1 month ago (2015-11-16 18:23:30 UTC) #18
Nico
On Tue, Nov 10, 2015 at 3:05 AM, <[email protected]> wrote: > On 2015/11/10 10:44:49, blundell ...
5 years, 1 month ago (2015-11-16 22:01:02 UTC) #19
blundell
On 2015/11/16 22:01:02, Nico (vacation Nov 18-19) wrote: > On Tue, Nov 10, 2015 at ...
5 years, 1 month ago (2015-11-20 11:31:04 UTC) #20
blundell
On 2015/11/16 22:01:02, Nico (vacation Nov 18-19) wrote: > On Tue, Nov 10, 2015 at ...
5 years, 1 month ago (2015-11-20 11:31:10 UTC) #21
blundell
On 2015/11/16 22:01:02, Nico wrote: > On Tue, Nov 10, 2015 at 3:05 AM, <mailto:[email protected]> ...
5 years ago (2015-11-25 09:45:58 UTC) #22
Nico
On Wed, Nov 25, 2015 at 4:45 AM, <[email protected]> wrote: > On 2015/11/16 22:01:02, Nico ...
5 years ago (2015-11-25 14:57:52 UTC) #23
blundell
On 2015/11/25 14:57:52, Nico wrote: > On Wed, Nov 25, 2015 at 4:45 AM, <mailto:[email protected]> ...
5 years ago (2015-11-30 08:36:39 UTC) #24
Nico
5 years ago (2015-12-01 15:53:30 UTC) #25
Message was sent while issue was closed.
On Mon, Nov 30, 2015 at 3:36 AM, <[email protected]> wrote:

> On 2015/11/25 14:57:52, Nico wrote:
>
> On Wed, Nov 25, 2015 at 4:45 AM, <mailto:[email protected]> wrote:
>>
>
> > On 2015/11/16 22:01:02, Nico wrote:
>> >
>> >> On Tue, Nov 10, 2015 at 3:05 AM, <mailto:[email protected]> wrote:
>> >>
>> >
>> > > On 2015/11/10 10:44:49, blundell wrote:
>> >> >
>> >> >> On 2015/11/10 10:43:45, blundell wrote:
>> >> >> > +thakis for //chrome
>> >> >>
>> >> >
>> >> > (note that the red bots are unrelated and due to gn's problem with
>> >> clobber
>> >> >> builds)
>> >> >>
>> >> >
>> >> > Nico,
>> >> >
>> >> > BTW, here were the results of my investigations of this issue with
>> gyp:
>> >> >
>> >> > - Remove out/Debug and run ./build/gyp_chromium && ninja -C out/Debug
>> >> > browser_sync_browser -> compilation fails due to the generated header
>> >> being
>> >> > missing
>> >> > - Do a successful build, run ninja -C out/Debug -t clean && ninja -C
>> >> > out/Debug
>> >> > browser_sync_browser -> compilation succeeds
>> >> >
>> >> > I'm not sure what the difference between the two cases is. I verified
>> >> that
>> >> > after
>> >> > cleaning the generated version_info_values.h file is no longer
>> present.
>> >> >
>> >>
>> >
>> > -t clean only removes files that ninja knows about, so it won't remove
>> >> outputs that get generated as a side effect but that aren't listed as
>> >> outputs in gyp/gn. That's the difference. I'm guessing something here
>> >> incorrectly depends on a file getting generated that isn't listed as an
>> >> output.
>> >>
>> >
>> > Please investigate this some more.
>> >>
>> >
>> >
>> > Hi Nico,
>> >
>> > I tracked this down. The "issue" is that when clang successfully builds
>> > profile_sync_service.cc, ninja saves the contents of the depfile that it
>> > asked
>> > clang to generate into .ninja_deps. That information includes the fact
>> that
>> > profile_sync_service.cc depends on version_info_values.h. On subsequent
>> > runs,
>> > ninja then uses that .ninja_deps file to figure out that it needs to
>> > regenerate
>> > version_info_values.h in order to rebuild profile_sync_service.cc.
>> >
>>
>
> Aha: the .d file tells ninja that the .cc file depends on the .h file and
>> you have an action that generates the .h file. From what you're saying, it
>> sounds like you have nothing else that makes the dependency on the .h
>> explicit in the build graph.
>>
>
>
> Correct.
>
> Since ninja can't know about .h dependencies before its first build, you
>> always need an explicit dependency from a target consuming generated .h
>> files to the target generating them. It sounds like this is what's missing
>> here.
>>
>
> Exactly.
>
>
> So I suppose the current behavior is WAI, but the intentions are not quite
>> right: You're missing a build dependency in your gyp / gn files :-)
>>
>
>
> I was not trying to imply that the missing dependency at the gyp/gn level
> was
> WAI (that's exactly what this CL is fixing in the first place


Ah, ok :-)


> , although I missed
> one dependency that I later discovered and handled in a followup). Rather,
> I'm
> saying that I guess it's WAI that *given that missing dependency* ninja has
> different output on its initial run than on any incremental run after a
> previous
> successful build (even e.g. a run after doing clean). That's what was
> surprising
> to me. Is ninja's behavior well-defined in the presence of missing
> dependencies,
> or is it kind of the wild west if you haven't specified the dependencies
> correctly?


If A depends on B, then ninja guarantees it will build B before A. If A and
B don't have a dependency between them and you ask ninja to build both,
then it makes no guarantees about which order it builds them in.


>
>
>
>
> >
>> > If I successfully build, then remove the generated version_info_values.h
>> > AND the
>> > .ninja_deps files, then rebuild browser_sync_browser, compilation fails
>> as
>> > expected.
>> >
>> > AFAICT this is WAI - it's just a fact of life that on the first build
>> the
>> > .ninja_deps file isn't present and so the dependency graph won't be the
>> > same as
>> > on an incremental build (obviously the fact that there was an
>> > underspecified
>> > dependency at the GYP level was not WAI, but I mean the fact that that
>> > underspecified dependency causes a problem for an initial build but not
>> an
>> > incremental build).
>> >
>> >
>> >
>> > >
>> >> > git cl try -c did not trigger the compile error, but I'm not sure
>> >> exactly
>> >> > what
>> >> > that does; does it just cause a clean build?
>> >> >
>> >> > https://codereview.chromium.org/1430393002/
>> >> >
>> >>
>> >
>> > --
>> >> You received this message because you are subscribed to the Google
>> Groups
>> >> "Chromium-reviews" group.
>> >> To unsubscribe from this group and stop receiving emails from it, send
>> an
>> >>
>> > email
>> >
>> >> to mailto:[email protected].
>> >>
>> >
>> >
>> >
>> > https://codereview.chromium.org/1430393002/
>> >
>>
>
> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:[email protected].
>>
>
>
>
> https://codereview.chromium.org/1430393002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].

Powered by Google App Engine
This is Rietveld 408576698