Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(251)

Issue 324403006: Add a new FORCE_DETERMINISTIC_BUILD define. (Closed)

Created:
6 years, 6 months ago by Sébastien Marchand
Modified:
6 years, 6 months ago
Reviewers:
brettw, Nico, M-A Ruel
CC:
chromium-reviews, erikwright+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

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

Patch Set 1 : #

Total comments: 6

Patch Set 2 : Address M-A's comments. #

Patch Set 3 : Address Brett's comment. #

Patch Set 4 : Move 'is_deterministic_build' to build\config\compiler\BUILD.gn #

Patch Set 5 : Move 'is_deterministic_build' to build\config\build.GN #

Patch Set 6 : Rename to force_deterministic_build #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -1 line) Patch
M build/common.gypi View 1 2 3 4 5 4 chunks +12 lines, -1 line 0 comments Download
M build/config/BUILD.gn View 1 2 3 4 5 2 chunks +9 lines, -0 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
Sébastien Marchand
PTAL.
6 years, 6 months ago (2014-06-11 19:52:08 UTC) #1
Sébastien Marchand
I think that there's something missing in my CL :), I use the 'deterministic_build' variable ...
6 years, 6 months ago (2014-06-11 19:58:08 UTC) #2
M-A Ruel
On 2014/06/11 19:58:08, Sébastien Marchand wrote: > I think that there's something missing in my ...
6 years, 6 months ago (2014-06-11 20:01:01 UTC) #3
Sébastien Marchand
Thanks, PTAnL. Brett, should I put some order in the 'declare_args' section ? There's a ...
6 years, 6 months ago (2014-06-11 20:25:41 UTC) #4
brettw
I'm curious about this feature. What is the scope of changes you expect? Is there ...
6 years, 6 months ago (2014-06-11 20:26:53 UTC) #5
Sébastien Marchand
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 ...
6 years, 6 months ago (2014-06-11 20:30:33 UTC) #6
brettw
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 ...
6 years, 6 months ago (2014-06-11 20:32:57 UTC) #7
Sébastien Marchand
Thanks, I've moved this to build\config\compiler\BUILD.gn
6 years, 6 months ago (2014-06-11 20:41:10 UTC) #8
brettw
In the GN build the build/config/BUILD.gn seemed more appropriate to me. What was the reason ...
6 years, 6 months ago (2014-06-11 20:42:16 UTC) #9
Sébastien Marchand
It's something that gets passed to the compiler, so I thought it was more appropriate ...
6 years, 6 months ago (2014-06-11 20:46:27 UTC) #10
brettw
Everything gets passed to the compiler :) This file is really for warnings and codegen ...
6 years, 6 months ago (2014-06-11 21:32:53 UTC) #11
Sébastien Marchand
Good point, I moved it back, PTAnL.
6 years, 6 months ago (2014-06-12 13:46:11 UTC) #12
M-A Ruel
lgtm
6 years, 6 months ago (2014-06-12 14:21:15 UTC) #13
M-A Ruel
On 2014/06/12 14:21:15, M-A Ruel wrote: > lgtm Actually, one thing I'm unsure; "is_deterministic_build" vs ...
6 years, 6 months ago (2014-06-12 14:22:44 UTC) #14
Sébastien Marchand
Sounds good, I've updated the CL.
6 years, 6 months ago (2014-06-12 14:37:03 UTC) #15
M-A Ruel
On 2014/06/12 14:37:03, Sébastien Marchand wrote: > Sounds good, I've updated the CL. More lgtm. ...
6 years, 6 months ago (2014-06-12 14:40:10 UTC) #16
brettw
lgtm
6 years, 6 months ago (2014-06-12 16:33:09 UTC) #17
Sébastien Marchand
The CQ bit was checked by sebmarchand@chromium.org
6 years, 6 months ago (2014-06-12 16:33:27 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/324403006/130001
6 years, 6 months ago (2014-06-12 16:36:02 UTC) #19
commit-bot: I haz the power
Change committed as 276801
6 years, 6 months ago (2014-06-12 21:01:25 UTC) #20
Nico
Why wouldn't builds just always be deterministic? Why do we need this flag?
6 years, 6 months ago (2014-06-12 21:05:58 UTC) #21
Nico
(i.e. do we really need GetBuildTime()?)
6 years, 6 months ago (2014-06-12 21:06:28 UTC) #22
M-A Ruel
6 years, 6 months ago (2014-06-12 21:08:54 UTC) #23
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698