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

Issue 1177163002: Fix gyp->make translation of rules with several outputs. (Closed)

Created:
5 years, 6 months ago by vogelheim
Modified:
5 years, 6 months ago
CC:
gyp-developer_googlegroups.com
Base URL:
https://chromium.googlesource.com/external/gyp@master
Target Ref:
refs/heads/master
Project:
gyp
Visibility:
Public.

Description

Fix gyp->make translation of rules with several outputs. This is effectively a follow-up to crrev.com/1066963002. The problem we have now run into is that, on some occasions, make would correctly build the 'lower' dependency, but would still not update the 'higher' dependency. The reason is that make could not 'see' that the intermediate target has been updated, and hence concluded that the 'lower' action hadn't done any actual work. Generally speaking, the fix is to make sure make has all the relevant information. Here, we make sure that: 1, the intermediate file is created. (do_cmd touch) 2, that is is created before the actual recipe is executed 3, that there is a recipe (which does nothing) for the true outputs. (This will force make to examine the actual file timestamp, meaning that it will 'see' the result of the actual actions performed.) The solution here still follows the 'intermediate' answer on stack overflow: http://stackoverflow.com/questions/2973445/gnu-makefile-rule-generating-a-few-targets-from-a-single-source-file These new additions follow a discussion thread on the GNU make mailing list, where the make maintainer explains the mechanism in more detail: http://lists.gnu.org/archive/html/help-make/2012-04/msg00015.html The effect of this bug is documented here: https://code.google.com/p/v8/issues/detail?id=4171 BUG= R=jochen@chromium.org Committed: https://chromium.googlesource.com/external/gyp/+/79de4031069f37870f04e8b6bbf42f0c7fec37aa

Patch Set 1 #

Patch Set 2 : Test case added. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -4 lines) Patch
M pylib/gyp/generator/make.py View 1 chunk +4 lines, -4 lines 0 comments Download
A test/actions-multiple-outputs-with-dependencies/gyptest-action.py View 1 1 chunk +38 lines, -0 lines 0 comments Download
A test/actions-multiple-outputs-with-dependencies/src/action.gyp View 1 1 chunk +28 lines, -0 lines 0 comments Download
A test/actions-multiple-outputs-with-dependencies/src/rcopy.py View 1 1 chunk +20 lines, -0 lines 0 comments Download

Messages

Total messages: 13 (5 generated)
commit-bot: I haz the power
Commit queue rejected this change because it did not recognize the base URL. Please commit ...
5 years, 6 months ago (2015-06-11 11:10:02 UTC) #3
vogelheim
5 years, 6 months ago (2015-06-11 11:44:04 UTC) #5
vogelheim
On 2015/06/11 11:44:04, vogelheim wrote: I don't think the trybots work for this repo. A ...
5 years, 6 months ago (2015-06-11 12:09:32 UTC) #6
jochen (gone - plz use gerrit)
is it possible to add a test for this?
5 years, 6 months ago (2015-06-11 12:37:48 UTC) #7
vogelheim
Test added.
5 years, 6 months ago (2015-06-12 10:12:11 UTC) #8
jochen (gone - plz use gerrit)
lgtm
5 years, 6 months ago (2015-06-12 10:31:12 UTC) #9
commit-bot: I haz the power
Commit queue rejected this change because it did not recognize the base URL. Please commit ...
5 years, 6 months ago (2015-06-12 12:00:03 UTC) #12
vogelheim
5 years, 6 months ago (2015-06-12 16:23:56 UTC) #13
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as
79de4031069f37870f04e8b6bbf42f0c7fec37aa (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698