Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 10388213: Don't generate LASTCHANGE at build time. (Closed)

Created:
6 years, 4 months ago by Torne
Modified:
6 years, 3 months ago
CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, dcaiafa+watch_chromium.org, wez+watch_chromium.org, erikwright (departed), amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, brettw-cc_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Remove rule entirely #

Patch Set 3 : Update export_tarball to run lastchange.py #

Total comments: 1

Patch Set 4 : Add comment to DEPS about keeping export_tarball current #

Patch Set 5 : Update copyright date in export_tarball #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -51 lines) Patch
M DEPS View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M base/base.gypi View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M build/all.gyp View 1 1 chunk +0 lines, -1 line 0 comments Download
M build/all_android.gyp View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
D build/util/build_util.gyp View 1 chunk +0 lines, -36 lines 0 comments Download
M chrome/chrome.gyp View 2 chunks +0 lines, -6 lines 0 comments Download
M chrome/chrome_installer.gypi View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M remoting/remoting.gyp View 1 2 3 1 chunk +0 lines, -3 lines 0 comments Download
M tools/export_tarball/export_tarball.py View 1 2 3 4 2 chunks +7 lines, -1 line 0 comments Download

Messages

Total messages: 24 (0 generated)
Torne
Hi Scott, I talked to you about the LASTCHANGE rule behaving weirdly back in January ...
6 years, 4 months ago (2012-05-21 17:36:01 UTC) #1
scottmg
On 2012/05/21 17:36:01, Torne wrote: > however, because the other targets that depended on it ...
6 years, 4 months ago (2012-05-21 17:52:28 UTC) #2
torne_google.com
On 21 May 2012 18:52, <scottmg@chromium.org> wrote: > On 2012/05/21 17:36:01, Torne wrote: > >> ...
6 years, 4 months ago (2012-05-22 13:23:19 UTC) #3
alexeypa (please no reviews)
On 2012/05/22 13:23:19, Torne (Richard Coles) wrote: > I don't know; unfortunately I think the ...
6 years, 4 months ago (2012-05-22 16:44:58 UTC) #4
Torne
On 2012/05/22 16:44:58, alexeypa wrote: > On 2012/05/22 13:23:19, Torne (Richard Coles) wrote: > > ...
6 years, 4 months ago (2012-05-22 16:47:32 UTC) #5
scottmg
On 2012/05/22 13:23:19, Torne (Richard Coles) wrote: > On 21 May 2012 18:52, <mailto:scottmg@chromium.org> wrote: ...
6 years, 4 months ago (2012-05-22 16:50:29 UTC) #6
torne_google.com
On 22 May 2012 17:50, <scottmg@chromium.org> wrote: > I wonder if the right thing to ...
6 years, 4 months ago (2012-05-22 16:53:58 UTC) #7
M-A Ruel
I don't have an opinion on this. 2012/5/22 Torne (Richard Coles) <torne@google.com>: > On 22 ...
6 years, 4 months ago (2012-05-22 20:22:31 UTC) #8
scottmg
On 2012/05/22 20:22:31, Marc-Antoine Ruel wrote: > I don't have an opinion on this. OK, ...
6 years, 4 months ago (2012-05-22 20:24:51 UTC) #9
Torne
On 2012/05/22 20:24:51, scottmg wrote: > On 2012/05/22 20:22:31, Marc-Antoine Ruel wrote: > > I ...
6 years, 4 months ago (2012-05-23 10:54:15 UTC) #10
scottmg
On 2012/05/23 10:54:15, Torne wrote: > On 2012/05/22 20:24:51, scottmg wrote: > > On 2012/05/22 ...
6 years, 4 months ago (2012-05-23 16:34:21 UTC) #11
Torne
On 2012/05/23 16:34:21, scottmg wrote: > On 2012/05/23 10:54:15, Torne wrote: > > On 2012/05/22 ...
6 years, 4 months ago (2012-05-23 16:38:25 UTC) #12
Torne
On 2012/05/23 16:38:25, Torne wrote: > No problem; I've found some of the details of ...
6 years, 4 months ago (2012-05-24 13:53:49 UTC) #13
scottmg
On 2012/05/24 13:53:49, Torne wrote: > I've uploaded the new patch to remove it entirely. ...
6 years, 4 months ago (2012-05-24 15:40:23 UTC) #14
Torne
Pawel, can you take a look at this? I've updated the export_tarball.py script to work, ...
6 years, 4 months ago (2012-05-25 13:01:08 UTC) #15
Paweł Hajdan Jr.
https://chromiumcodereview.appspot.com/10388213/diff/18001/tools/export_tarball/export_tarball.py File tools/export_tarball/export_tarball.py (right): https://chromiumcodereview.appspot.com/10388213/diff/18001/tools/export_tarball/export_tarball.py#newcode104 tools/export_tarball/export_tarball.py:104: if subprocess.call(['python', 'build/util/lastchange.py', '-o', This is introducing code duplication. ...
6 years, 4 months ago (2012-05-25 18:49:35 UTC) #16
Torne
On 2012/05/25 18:49:35, Paweł Hajdan Jr. wrote: > https://chromiumcodereview.appspot.com/10388213/diff/18001/tools/export_tarball/export_tarball.py > File tools/export_tarball/export_tarball.py (right): > > ...
6 years, 4 months ago (2012-05-25 23:16:02 UTC) #17
Paweł Hajdan Jr.
On 2012/05/25 23:16:02, Torne wrote: > On 2012/05/25 18:49:35, Paweł Hajdan Jr. wrote: > > ...
6 years, 3 months ago (2012-05-29 14:42:21 UTC) #18
Torne
On 2012/05/29 14:42:21, Paweł Hajdan Jr. wrote: > On 2012/05/25 23:16:02, Torne wrote: > > ...
6 years, 3 months ago (2012-05-29 14:56:46 UTC) #19
Torne
Alex, Brett, can I get OWNERS approval for the gyp files in remoting and base ...
6 years, 3 months ago (2012-05-30 09:55:34 UTC) #20
brettw
lgtm
6 years, 3 months ago (2012-05-30 15:18:22 UTC) #21
alexeypa (please no reviews)
lgtm remoting/* I agree that LASTCHANGE generation does not belong to the build scripts. Ideally ...
6 years, 3 months ago (2012-05-30 15:23:09 UTC) #22
Paweł Hajdan Jr.
LGTM
6 years, 3 months ago (2012-05-30 17:16:41 UTC) #23
Torne
6 years, 3 months ago (2012-05-30 17:24:50 UTC) #24
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.

Powered by Google App Engine
This is Rietveld 408576698