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

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

Created:
6 years, 1 month ago by Torne
Modified:
6 years, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month ago (2012-05-30 09:55:34 UTC) #20
brettw
lgtm
6 years, 1 month 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, 1 month ago (2012-05-30 15:23:09 UTC) #22
Paweł Hajdan Jr.
LGTM
6 years, 1 month ago (2012-05-30 17:16:41 UTC) #23
Torne
6 years, 1 month 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