Don't generate LASTCHANGE at build time.

The LASTCHANGE file is created and updated by gclient runhooks under
normal circumstances, so it makes more sense to treat it as if it were a
checked-in source file, not a generated file. The rule for its
generation was still getting run once on a clean build, because the
other targets that depended on it expected a gyp timestamp file to exist
for the target.

There's no need for any other targets to actually depend on lastchange
(and in fact several of the targets that make use of the file already
don't depend on the target); it should already exist, and if it doesn't
then the source checkout is probably broken (as the other steps in
runhooks are also mandatory). So, remove the rule entirely.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=139765

[Torne, 2012-05-21 17:36:01]

Hi Scott,

I talked to you about the LASTCHANGE rule behaving weirdly back in January but
couldn't come up with a way to solve it; does this patch look reasonable? The
reason I'm looking at this again now is it's causing some problems in our
downstream (internal) android port build because of the currently suboptimal way
we manage our source tree.

I've tested this on Linux with make and ninja and the behaviour is "correct": it
now runs lastchange.py precisely once when LASTCHANGE doesn't exist, and never
under any other circumstances (whereas previously it ran it after each time you
rm -rf out). I'm not sure if this works on MSVC/Xcode; I will test this, but
wanted to see what you thought of the patch anyway.

Thanks!

[scottmg, 2012-05-21 17:52:28]

On 2012/05/21 17:36:01, Torne wrote:

> however, because the other targets that depended on it
> expected a gyp timestamp file to exist for the target.

Sorry, I don't understand that part from the description, could you try to
explain more?

I guess the part I'm missing is: why does depending on the target change whether
there's an extra execution of the extraction step?

Also, will the ordering be correct in VS builds if they're just referencing that
file without depending on the target?

[torne_google.com, 2012-05-22 13:23:19]

On 21 May 2012 18:52,  <scottmg@chromium.org> wrote:
> On 2012/05/21 17:36:01, Torne wrote:
>
>> however, because the other targets that depended on it
>> expected a gyp timestamp file to exist for the target.
>
>
> Sorry, I don't understand that part from the description, could you try to
> explain more?
>
> I guess the part I'm missing is: why does depending on the target change
> whether
> there's an extra execution of the extraction step?

When some target foo depends on build/util/build_util.gyp:lastchange
it means that in order for foo to be built,
out/Debug/build/util/lastchange.stamp must exist (at least, in the
make and ninja build systems). This is not the case in a clean build,
because nothing in out/ exists, so the lastchange target gets run even
though the LASTCHANGE file already exists.

When *no* target depends on build/util/build_util.gyp:lastchange, then
nothing ever expects that timestamp file to be created, but in make
and ninja the *rule* that tells it how to create the LASTCHANGE file
still exists, so if the file is missing it will execute the command
and create it.

> Also, will the ordering be correct in VS builds if they're just referencing
> that
> file without depending on the target?

I don't know; unfortunately I think the answer is no, and this
workaround will break VS and XCode builds (in the case where
LASTCHANGE doesn't exist; it will work fine if runhooks already
created it). I haven't had a chance to test it.

> https://chromiumcodereview.appspot.com/10388213/



-- 
Torne (Richard Coles)
torne@google.com

[alexeypa (please no reviews), 2012-05-22 16:44:58]

On 2012/05/22 13:23:19, Torne (Richard Coles) wrote:
> I don't know; unfortunately I think the answer is no, and this
> workaround will break VS and XCode builds (in the case where
> LASTCHANGE doesn't exist; it will work fine if runhooks already
> created it). I haven't had a chance to test it.

I tested it on VS2010. Deleting LASTCHANGE breaks the build. Running "gclient
runhooks" restores LASTCHANGE allowing the build to proceed without any issues.

[Torne, 2012-05-22 16:47:32]

On 2012/05/22 16:44:58, alexeypa wrote:
> On 2012/05/22 13:23:19, Torne (Richard Coles) wrote:
> > I don't know; unfortunately I think the answer is no, and this
> > workaround will break VS and XCode builds (in the case where
> > LASTCHANGE doesn't exist; it will work fine if runhooks already
> > created it). I haven't had a chance to test it.
> 
> I tested it on VS2010. Deleting LASTCHANGE breaks the build. Running "gclient
> runhooks" restores LASTCHANGE allowing the build to proceed without any
issues.

Thanks for trying this; I didn't really think the win/mac situation through when
coming up with this, and yeah, it doesn't work because there's no global rule
database in those build systems and thus no magic cross-target file
dependencies.

Is the case where LASTCHANGE has been deleted even interesting? Would anyone be
sad if that didn't work on any platform? (i.e. delete the rule entirely).

[scottmg, 2012-05-22 16:50:29]

On 2012/05/22 13:23:19, Torne (Richard Coles) wrote:
> On 21 May 2012 18:52,  <mailto:scottmg@chromium.org> wrote:
> > On 2012/05/21 17:36:01, Torne wrote:
> >
> >> however, because the other targets that depended on it
> >> expected a gyp timestamp file to exist for the target.
> >
> >
> > Sorry, I don't understand that part from the description, could you try to
> > explain more?
> >
> > I guess the part I'm missing is: why does depending on the target change
> > whether
> > there's an extra execution of the extraction step?
> 
> When some target foo depends on build/util/build_util.gyp:lastchange
> it means that in order for foo to be built,
> out/Debug/build/util/lastchange.stamp must exist (at least, in the
> make and ninja build systems). This is not the case in a clean build,
> because nothing in out/ exists, so the lastchange target gets run even
> though the LASTCHANGE file already exists.

Gotcha, thanks.

> 
> When *no* target depends on build/util/build_util.gyp:lastchange, then
> nothing ever expects that timestamp file to be created, but in make
> and ninja the *rule* that tells it how to create the LASTCHANGE file
> still exists, so if the file is missing it will execute the command
> and create it.
> 
> > Also, will the ordering be correct in VS builds if they're just referencing
> > that
> > file without depending on the target?
> 
> I don't know; unfortunately I think the answer is no, and this
> workaround will break VS and XCode builds (in the case where
> LASTCHANGE doesn't exist; it will work fine if runhooks already
> created it). I haven't had a chance to test it.

I wonder if the right thing to do then is simply to remove LASTCHANGE generation
from gyp entirely. i.e. say that it must be extracted by gclient runhooks, and
is never generated by the build.

I think M-A didn't like that idea when we last talked about it, but I forget the
rationale.


> > https://chromiumcodereview.appspot.com/10388213/
> 
> 
> 
> -- 
> Torne (Richard Coles)
> mailto:torne@google.com

[torne_google.com, 2012-05-22 16:53:58]

On 22 May 2012 17:50,  <scottmg@chromium.org> wrote:
> I wonder if the right thing to do then is simply to remove LASTCHANGE
> generation
> from gyp entirely. i.e. say that it must be extracted by gclient runhooks,
> and
> is never generated by the build.
>
> I think M-A didn't like that idea when we last talked about it, but I forget
> the
> rationale.

I think this might be the right thing to do, yes. If you're checking
out the source from our tree, then you should not expect the source to
build if you haven't run runhooks, because it also does other stuff
that's mandatory for the build to work. If you're getting the source
from a tarball or something, then lastchange.py is not going to be
able to generate a meaningful number anyway, and probably the right
thing to do is to arrange for the script that generated that tarball
to package LASTCHANGE as well, such that the tarball-user *does* get
the right SVN revision in their build even though they didn't check it
out from SVN.

I'll happily believe there's an important case I haven't thought of, though :)

-- 
Torne (Richard Coles)
torne@google.com

[M-A Ruel, 2012-05-22 20:22:31]

I don't have an opinion on this.

2012/5/22 Torne (Richard Coles) <torne@google.com>:
> On 22 May 2012 17:50,  <scottmg@chromium.org> wrote:
>> I wonder if the right thing to do then is simply to remove LASTCHANGE
>> generation
>> from gyp entirely. i.e. say that it must be extracted by gclient runhooks,
>> and
>> is never generated by the build.
>>
>> I think M-A didn't like that idea when we last talked about it, but I forget
>> the
>> rationale.
>
> I think this might be the right thing to do, yes. If you're checking
> out the source from our tree, then you should not expect the source to
> build if you haven't run runhooks, because it also does other stuff
> that's mandatory for the build to work. If you're getting the source
> from a tarball or something, then lastchange.py is not going to be
> able to generate a meaningful number anyway, and probably the right
> thing to do is to arrange for the script that generated that tarball
> to package LASTCHANGE as well, such that the tarball-user *does* get
> the right SVN revision in their build even though they didn't check it
> out from SVN.
>
> I'll happily believe there's an important case I haven't thought of, though
:)
>
> --
> Torne (Richard Coles)
> torne@google.com

[scottmg, 2012-05-22 20:24:51]

On 2012/05/22 20:22:31, Marc-Antoine Ruel wrote:
> I don't have an opinion on this.

OK, my mistake. I think nuking it entirely is the right way to go then torne.
(There may be some fallout, but seems worth trying if it fixes some android
problems)

[Torne, 2012-05-23 10:54:15]

On 2012/05/22 20:24:51, scottmg wrote:
> On 2012/05/22 20:22:31, Marc-Antoine Ruel wrote:
> > I don't have an opinion on this.
> 
> OK, my mistake. I think nuking it entirely is the right way to go then torne.

I've found one possible issue with nuking it entirely: currently the tarball
source snapshots on http://gsdview.appspot.com/chromium-browser-official/ don't
contain build/util/LASTCHANGE - i.e. they haven't had runhooks called on them.
I'm not sure how these get generated, but it seems like they should probably
contain this, as I'd assume that's the only way for builds from that source to
know what svn revision they are based on.

> (There may be some fallout, but seems worth trying if it fixes some android
> problems)

It lets us check a LASTCHANGE file into our downstream copy of chromium during
merges, so that we actually get the right svn revision and it doesn't get
overwritten by lastchange.py, yes :)

[scottmg, 2012-05-23 16:34:21]

On 2012/05/23 10:54:15, Torne wrote:
> On 2012/05/22 20:24:51, scottmg wrote:
> > On 2012/05/22 20:22:31, Marc-Antoine Ruel wrote:
> > > I don't have an opinion on this.
> > 
> > OK, my mistake. I think nuking it entirely is the right way to go then
torne.
> 
> I've found one possible issue with nuking it entirely: currently the tarball
> source snapshots on http://gsdview.appspot.com/chromium-browser-official/
don't
> contain build/util/LASTCHANGE - i.e. they haven't had runhooks called on them.
> I'm not sure how these get generated, but it seems like they should probably
> contain this, as I'd assume that's the only way for builds from that source to
> know what svn revision they are based on.

OK. I'm afraid I don't know whether that's important or who would need to fix a
packaging step to make it work. Those snapshots don't seem to be accessible
right now so I couldn't look at one.

More generally, I don't know if there's other processes that try to build
without having done runhooks. But like you say, I think the tree's effectively
broken without having done that. For example, there wouldn't be a nacl
toolchain. So, it seems fine to me to remove the build rules and only have
LASTCHANGE generated in runhooks, if you want to change this CL to do that.

(Sorry this is taking so long)

[Torne, 2012-05-23 16:38:25]

On 2012/05/23 16:34:21, scottmg wrote:
> On 2012/05/23 10:54:15, Torne wrote:
> > On 2012/05/22 20:24:51, scottmg wrote:
> > > On 2012/05/22 20:22:31, Marc-Antoine Ruel wrote:
> > > > I don't have an opinion on this.
> > > 
> > > OK, my mistake. I think nuking it entirely is the right way to go then
> torne.
> > 
> > I've found one possible issue with nuking it entirely: currently the tarball
> > source snapshots on http://gsdview.appspot.com/chromium-browser-official/
> don't
> > contain build/util/LASTCHANGE - i.e. they haven't had runhooks called on
them.
> > I'm not sure how these get generated, but it seems like they should probably
> > contain this, as I'd assume that's the only way for builds from that source
to
> > know what svn revision they are based on.
> 
> OK. I'm afraid I don't know whether that's important or who would need to fix
a
> packaging step to make it work. Those snapshots don't seem to be accessible
> right now so I couldn't look at one.
> 
> More generally, I don't know if there's other processes that try to build
> without having done runhooks. But like you say, I think the tree's effectively
> broken without having done that. For example, there wouldn't be a nacl
> toolchain. So, it seems fine to me to remove the build rules and only have
> LASTCHANGE generated in runhooks, if you want to change this CL to do that.
> 
> (Sorry this is taking so long)

No problem; I've found some of the details of the snapshot generation, and am
going to talk to the relevant people about it. There isn't a rush. I'll upload a
new patch that removes the rule entirely, but have been too busy with other
things today.

[Torne, 2012-05-24 13:53:49]

On 2012/05/23 16:38:25, Torne wrote:
> No problem; I've found some of the details of the snapshot generation, and am
> going to talk to the relevant people about it. There isn't a rush. I'll upload
a
> new patch that removes the rule entirely, but have been too busy with other
> things today.

I've uploaded the new patch to remove it entirely. I'm still going to check up
how the snapshot gets generated before submitting this.

[scottmg, 2012-05-24 15:40:23]

On 2012/05/24 13:53:49, Torne wrote:
> I've uploaded the new patch to remove it entirely. I'm still going to check up
> how the snapshot gets generated before submitting this.

lgtm

This was the problem I ran into
http://code.google.com/p/chromium/issues/detail?id=112264 with the previous
change. I'd suggest keeping an eye on the Offical builders as they seem most
likely to have unusual behaviour here.

[Torne, 2012-05-25 13:01:08]

Pawel, can you take a look at this? I've updated the export_tarball.py script to
work, and it seems to be okay if I run it locally, but I'm not sure what
environment it runs in on the bots (which bot does it even run on?).

[Paweł Hajdan Jr., 2012-05-25 18:49:35]

https://chromiumcodereview.appspot.com/10388213/diff/18001/tools/export_tarba...
File tools/export_tarball/export_tarball.py (right):

https://chromiumcodereview.appspot.com/10388213/diff/18001/tools/export_tarba...
tools/export_tarball/export_tarball.py:104: if subprocess.call(['python',
'build/util/lastchange.py', '-o',
This is introducing code duplication. I appreciate that you remember about
export_tarball.py (thanks!), but I'm afraid most people have no idea it even
exists.

Will this really result in build errors in build/util/LASTCHANGE file is not
present? If so, I'd prefer making that non-fatal.

[Torne, 2012-05-25 23:16:02]

On 2012/05/25 18:49:35, Paweł Hajdan Jr. wrote:
>
https://chromiumcodereview.appspot.com/10388213/diff/18001/tools/export_tarba...
> File tools/export_tarball/export_tarball.py (right):
> 
>
https://chromiumcodereview.appspot.com/10388213/diff/18001/tools/export_tarba...
> tools/export_tarball/export_tarball.py:104: if subprocess.call(['python',
> 'build/util/lastchange.py', '-o',
> This is introducing code duplication. I appreciate that you remember about
> export_tarball.py (thanks!), but I'm afraid most people have no idea it even
> exists.

I realise this is duplicated. The alternative would be to just invoke gclient
runhooks in export_tarball, I guess? (or to have the bot that runs it allow the
hooks to run when it syncs). Where does this run?

> Will this really result in build errors in build/util/LASTCHANGE file is not
> present? If so, I'd prefer making that non-fatal.

Yes, it will result in make (or ninja) failing because there is no rule to
create build/util/LASTCHANGE. This cannot be made non-fatal easily (or possibly
at all?) - it's not some tool objecting, it's make itself. The rule to generate
the version header must depend on LASTCHANGE as an input, because otherwise it
would not get regenerated when LASTCHANGE changes, but this means that make
cannot proceed if the file does not exist. I can't think of a workaround for
this in gyp; my original patch here attempted to solve it without breaking the
case where LASTCHANGE is absent but that does not work for MSVC (and probably
not for xcode either).

The goal of removing the rule is specifically for the benefit of things like the
export_tarball script, in fact: currently the tarball will get a LASTCHANGE of
0, because when the lastchange.py script runs at build time it will find it is
not in a svn/git checkout and output 0. By eliminating the build rule it's
possible to just include the correct LASTCHANGE file in the tarball. My specific
case is similar: Chrome for Android is not yet upstreamed and currently exists
as a fork in an internal repository, and lastchange.py cannot be relied on to
generate the correct version number because of cherry-picks present in our
history. So, we would like to commit the correct LASTCHANGE  file during our
merge-from-upstream process, but this is not possible while it still has a build
rule.

[Paweł Hajdan Jr., 2012-05-29 14:42:21]

On 2012/05/25 23:16:02, Torne wrote:
> On 2012/05/25 18:49:35, Paweł Hajdan Jr. wrote:
> >
>
https://chromiumcodereview.appspot.com/10388213/diff/18001/tools/export_tarba...
> > File tools/export_tarball/export_tarball.py (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10388213/diff/18001/tools/export_tarba...
> > tools/export_tarball/export_tarball.py:104: if subprocess.call(['python',
> > 'build/util/lastchange.py', '-o',
> > This is introducing code duplication. I appreciate that you remember about
> > export_tarball.py (thanks!), but I'm afraid most people have no idea it even
> > exists.
> 
> I realise this is duplicated. The alternative would be to just invoke gclient
> runhooks in export_tarball, I guess? (or to have the bot that runs it allow
the
> hooks to run when it syncs). Where does this run?

We aren't going to run hooks in export_tarball or at any other point of
generating the tarball. The bot does not have all dependencies needed for gyp,
and we don't want to ship Makefiles in the tarball.

At the very least update DEPS file to say above the LASTCHANGE hook to also
update export_tarball when the hook is changed.

> The goal of removing the rule is specifically for the benefit of things like
the
> export_tarball script, in fact: currently the tarball will get a LASTCHANGE of
> 0, because when the lastchange.py script runs at build time it will find it is
> not in a svn/git checkout and output 0. By eliminating the build rule it's
> possible to just include the correct LASTCHANGE file in the tarball.

That was never a concern for me (FYI).

> My specific
> case is similar: Chrome for Android is not yet upstreamed and currently exists
> as a fork in an internal repository, and lastchange.py cannot be relied on to
> generate the correct version number because of cherry-picks present in our
> history. So, we would like to commit the correct LASTCHANGE  file during our
> merge-from-upstream process, but this is not possible while it still has a
build
> rule.

Are you sure checking in LASTCHANGE is absolutely the right thing to do here?
I'm not going to block anything on this one (my comments are above), but it
seems a bit strange. It would be interesting to know more about the problem
you're solving.

[Torne, 2012-05-29 14:56:46]

On 2012/05/29 14:42:21, Paweł Hajdan Jr. wrote:
> On 2012/05/25 23:16:02, Torne wrote:
> > On 2012/05/25 18:49:35, Paweł Hajdan Jr. wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/10388213/diff/18001/tools/export_tarba...
> > > File tools/export_tarball/export_tarball.py (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10388213/diff/18001/tools/export_tarba...
> > > tools/export_tarball/export_tarball.py:104: if subprocess.call(['python',
> > > 'build/util/lastchange.py', '-o',
> > > This is introducing code duplication. I appreciate that you remember about
> > > export_tarball.py (thanks!), but I'm afraid most people have no idea it
even
> > > exists.
> > 
> > I realise this is duplicated. The alternative would be to just invoke
gclient
> > runhooks in export_tarball, I guess? (or to have the bot that runs it allow
> the
> > hooks to run when it syncs). Where does this run?
> 
> We aren't going to run hooks in export_tarball or at any other point of
> generating the tarball. The bot does not have all dependencies needed for gyp,
> and we don't want to ship Makefiles in the tarball.

OK.

> At the very least update DEPS file to say above the LASTCHANGE hook to also
> update export_tarball when the hook is changed.

I can do that, yes.

> > The goal of removing the rule is specifically for the benefit of things like
> the
> > export_tarball script, in fact: currently the tarball will get a LASTCHANGE
of
> > 0, because when the lastchange.py script runs at build time it will find it
is
> > not in a svn/git checkout and output 0. By eliminating the build rule it's
> > possible to just include the correct LASTCHANGE file in the tarball.
> 
> That was never a concern for me (FYI).

Sure, but it seems like a logical thing to have in there if it can be done.

> > My specific
> > case is similar: Chrome for Android is not yet upstreamed and currently
exists
> > as a fork in an internal repository, and lastchange.py cannot be relied on
to
> > generate the correct version number because of cherry-picks present in our
> > history. So, we would like to commit the correct LASTCHANGE  file during our
> > merge-from-upstream process, but this is not possible while it still has a
> build
> > rule.
> 
> Are you sure checking in LASTCHANGE is absolutely the right thing to do here?
> I'm not going to block anything on this one (my comments are above), but it
> seems a bit strange. It would be interesting to know more about the problem
> you're solving.

The problem is that we are building in an environment where lastchange.py
returns the wrong revision number most of the time (because the tree is a
downstream fork managed with git that contains cherrypicks of SVN commits from
the "future"). We run a script regularly to merge changes from chromium trunk
into our tree; this script is the only thing that reliably knows what SVN
revision we have merged up to, which is the number that we would prefer to have
in LASTCHANGE. We are also not supposed to be writing any output to the source
tree in our environment, and moving LASTCHANGE to be under out/ looks odd on the
Chromium build..

Checking in a LASTCHANGE file seems like the easiest way to make this work;
unlike in a regular chromium checkout where LASTCHANGE changes with every commit
to trunk, ours changes ~weekly and can be updated during an already-existing
merge commit.

I realise it's possible to handle all of this by making lastchange generation
more complicated; I believe this CL makes it simpler and still solves the
problem.

[Torne, 2012-05-30 09:55:34]

Alex, Brett, can I get OWNERS approval for the gyp files in remoting and base
respectively?

(though do feel free to comment on whether you think this is a good idea
also...) :)

[brettw, 2012-05-30 15:18:22]

lgtm

[alexeypa (please no reviews), 2012-05-30 15:23:09]

lgtm remoting/*

I agree that LASTCHANGE generation does not belong to the build scripts. Ideally
it should be updated at the time of making a local copy of the sources (being
that checking out SVN, unpacking tarball or something else).

[Paweł Hajdan Jr., 2012-05-30 17:16:41]

LGTM

[Torne, 2012-05-30 17:24:50]

Thanks all; I will submit this in the (European) morning when I have time to
watch the tree and there are less commits. I'll make sure the bots are still
generating the correct LASTCHANGE.