Add a new FORCE_DETERMINISTIC_BUILD define.

Add a new FORCE_DETERMINISTIC_BUILD define.

This flag will be used to make the builds determinist, e.g. in place like this one: https://code.google.com/p/chromium/codesearch#chromium/src/base/build_time.h&l=15

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

[Sébastien Marchand, 2014-06-11 19:52:08]

PTAL.

[Sébastien Marchand, 2014-06-11 19:58:08]

I think that there's something missing in my CL :), I use the
'deterministic_build' variable in BUILD.gn but I don't set it anywhere... For
GYP it comes from the GYP_DEFINES, is there a similar variable with GN ?

[M-A Ruel, 2014-06-11 20:01:01]

On 2014/06/11 19:58:08, Sébastien Marchand wrote:
> I think that there's something missing in my CL :), I use the
> 'deterministic_build' variable in BUILD.gn but I don't set it anywhere... For
> GYP it comes from the GYP_DEFINES, is there a similar variable with GN ?

Yes, you have to declare the default value in BUILDCONFIG.gn.

Also update the BUG=314403
It's the meta bug but it's the closest one for now, unless you feel like filing
more bugs. :)

https://codereview.chromium.org/324403006/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/324403006/diff/40001/build/common.gypi#newcod...
build/common.gypi:277: #     deterministic build.
Add http://crbug.com/314403

https://codereview.chromium.org/324403006/diff/40001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/324403006/diff/40001/build/config/BUILD.gn#ne...
build/config/BUILD.gn:56: defines += [ "DETERMINISTIC_BUILD=1" ]
Why do you set it to 1 here and no in common.gypi. An oversight?

[Sébastien Marchand, 2014-06-11 20:25:41]

Thanks, PTAnL.

Brett, should I put some order in the 'declare_args' section ? There's a 'KEEP
IN ALPHABETICAL ORDER' in top of it but this isn't in alphabetical order... I
don't mind cleaning this in this CL. However this'll break the 'groups' of
variables (e.g. the 'is_*san' ones).

https://codereview.chromium.org/324403006/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/324403006/diff/40001/build/common.gypi#newcod...
build/common.gypi:277: #     deterministic build.
On 2014/06/11 20:01:00, M-A Ruel wrote:
> Add http://crbug.com/314403

Done.

https://codereview.chromium.org/324403006/diff/40001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/324403006/diff/40001/build/config/BUILD.gn#ne...
build/config/BUILD.gn:56: defines += [ "DETERMINISTIC_BUILD=1" ]
On 2014/06/11 20:01:01, M-A Ruel wrote:
> Why do you set it to 1 here and no in common.gypi. An oversight?

Because I was scared by GN so I haven't realized that I was setting it
differently... :) (cut&paste error)

[brettw, 2014-06-11 20:26:53]

I'm curious about this feature. What is the scope of changes you expect? Is
there a design doc or anything like that?

https://codereview.chromium.org/324403006/diff/40001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/324403006/diff/40001/build/config/BUILD.gn#ne...
build/config/BUILD.gn:55: if (deterministic_build) {
You need to declare this as an "Arg" like disable_iterator_debugging at the top,
and give it documentation and a default value. As is this will fail to run since
its not defined.

[Sébastien Marchand, 2014-06-11 20:30:33]

Here's the design doc for this:
http://www.chromium.org/developers/testing/isolated-testing/deterministic-builds

And the tracking bug: https://code.google.com/p/chromium/issues/detail?id=314403

https://codereview.chromium.org/324403006/diff/40001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/324403006/diff/40001/build/config/BUILD.gn#ne...
build/config/BUILD.gn:55: if (deterministic_build) {
On 2014/06/11 20:26:53, brettw wrote:
> You need to declare this as an "Arg" like disable_iterator_debugging at the
top,
> and give it documentation and a default value. As is this will fail to run
since
> its not defined.

Is it ok to define it in BUILDCONFIG.gn ?

[brettw, 2014-06-11 20:32:57]

On 2014/06/11 20:30:33, Sébastien Marchand wrote:
> Here's the design doc for this:
>
http://www.chromium.org/developers/testing/isolated-testing/deterministic-builds
> 
> And the tracking bug:
https://code.google.com/p/chromium/issues/detail?id=314403
> 
> https://codereview.chromium.org/324403006/diff/40001/build/config/BUILD.gn
> File build/config/BUILD.gn (right):
> 
>
https://codereview.chromium.org/324403006/diff/40001/build/config/BUILD.gn#ne...
> build/config/BUILD.gn:55: if (deterministic_build) {
> On 2014/06/11 20:26:53, brettw wrote:
> > You need to declare this as an "Arg" like disable_iterator_debugging at the
> top,
> > and give it documentation and a default value. As is this will fail to run
> since
> > its not defined.
> 
> Is it ok to define it in BUILDCONFIG.gn ?

No, things should be scoped as narrowly as possible (think C++).

[Sébastien Marchand, 2014-06-11 20:41:10]

Thanks, I've moved this to build\config\compiler\BUILD.gn

[brettw, 2014-06-11 20:42:16]

In the GN build the build/config/BUILD.gn seemed more appropriate to me. What
was the reason for the move?

[Sébastien Marchand, 2014-06-11 20:46:27]

It's something that gets passed to the compiler, so I thought it was more
appropriate to put this in the compiler file.
Le 2014-06-11 16:42, <brettw@chromium.org> a écrit :

> In the GN build the build/config/BUILD.gn seemed more appropriate to me.
> What
> was the reason for the move?
>
> https://codereview.chromium.org/324403006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2014-06-11 21:32:53]

Everything gets passed to the compiler :) This file is really for warnings and
codegen stuff.

[Sébastien Marchand, 2014-06-12 13:46:11]

Good point, I moved it back, PTAnL.

[M-A Ruel, 2014-06-12 14:21:15]

lgtm

[M-A Ruel, 2014-06-12 14:22:44]

On 2014/06/12 14:21:15, M-A Ruel wrote:
> lgtm

Actually, one thing I'm unsure;

"is_deterministic_build" vs maybe something like "force_deterministic_build".
There's a subtle difference in the meantime, which could help the reader to
better understand the scope of this flag.

So the define should probably be changed too, so that when reading something
like:
#if defined(FORCE_DETERMINISTIC_BUILD)

it is obvious why and what is being done.

Any opinion?

[Sébastien Marchand, 2014-06-12 14:37:03]

Sounds good, I've updated the CL.

[M-A Ruel, 2014-06-12 14:40:10]

On 2014/06/12 14:37:03, Sébastien Marchand wrote:
> Sounds good, I've updated the CL.

More lgtm. I think it's now much more self-documenting. Let's wait for Brett's
opinion.

[brettw, 2014-06-12 16:33:09]

lgtm

[Sébastien Marchand, 2014-06-12 16:33:27]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2014-06-12 16:36:02]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/324403006/130001

[commit-bot: I haz the power, 2014-06-12 21:01:25]

Change committed as 276801

[Nico, 2014-06-12 21:05:58]

Why wouldn't builds just always be deterministic? Why do we need this flag?

[Nico, 2014-06-12 21:06:28]

(i.e. do we really need GetBuildTime()?)

[M-A Ruel, 2014-06-12 21:08:54]

On 2014/06/12 21:06:28, Nico (away) wrote:
> (i.e. do we really need GetBuildTime()?)

That's a fair question but there will be more implications like not saving the
svn version in Chrome itself. As such, we prefer to not make it the default for
now.