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

Issue 9121011: ninja/mac: Don't choke on bundles that have no 'sources'. (Closed)

Created:
8 years, 11 months ago by Nico
Modified:
7 years, 3 months ago
Reviewers:
Evan Martin
CC:
gyp-developer_googlegroups.com, jeremya
Visibility:
Public.

Description

ninja/mac: Don't choke on bundles that have no 'sources'. Also support completely empty bundles. One problem with supporting sourceless bundles was that the executable build rule always used final_deps as input, but if there are no sources, then final_deps was equal to source_depends or action_depends. Distinguish between link_deps and final_deps, and just write a stamp file instead of creating a binary (without sources). Sourceless non-bundle (non-'none') targets with actions are still broken; but they're broken in the make generator too, so they're apparently not used. I will make input.py error out on these in a later CL. BUG=96894 Committed: http://code.google.com/p/gyp/source/detail?r=1136

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : Not done yet, added failing test. #

Patch Set 7 : actual test #

Total comments: 1

Patch Set 8 : passes #

Patch Set 9 : '' #

Patch Set 10 : '' #

Patch Set 11 : '' #

Patch Set 12 : expand test; fails again #

Patch Set 13 : expand test; fails again #

Total comments: 2

Patch Set 14 : comment #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+129 lines, -29 lines) Patch
M pylib/gyp/generator/ninja.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 10 chunks +43 lines, -29 lines 2 comments Download
A test/mac/gyptest-sourceless-module.gyp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +46 lines, -0 lines 1 comment Download
A test/mac/sourceless-module/empty.c View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
A test/mac/sourceless-module/test.gyp View 1 2 3 4 5 1 chunk +39 lines, -0 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
Nico
The completely empty bundles aren't needed by the chromium build, but I found that ninja ...
8 years, 11 months ago (2012-01-07 00:14:16 UTC) #1
Nico
Actually, don't review this yet. I added an additional test (reduced from chromium) which doesn't ...
8 years, 11 months ago (2012-01-07 00:56:36 UTC) #2
Nico
Ok, this is now ready.
8 years, 11 months ago (2012-01-07 06:32:13 UTC) #3
Evan Martin
http://codereview.chromium.org/9121011/diff/3006/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): http://codereview.chromium.org/9121011/diff/3006/pylib/gyp/generator/ninja.py#newcode296 pylib/gyp/generator/ninja.py:296: self.ninja.build(self.name, 'phony', output) Would reusing the above similar line ...
8 years, 11 months ago (2012-01-07 21:10:12 UTC) #4
Nico
http://codereview.chromium.org/9121011/diff/7006/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): http://codereview.chromium.org/9121011/diff/7006/pylib/gyp/generator/ninja.py#newcode293 pylib/gyp/generator/ninja.py:293: self.ninja.build(self.name, 'phony', output) On 2012/01/07 21:10:12, Evan Martin wrote: ...
8 years, 11 months ago (2012-01-07 21:26:20 UTC) #5
Evan Martin
http://codereview.chromium.org/9121011/diff/12001/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): http://codereview.chromium.org/9121011/diff/12001/pylib/gyp/generator/ninja.py#newcode291 pylib/gyp/generator/ninja.py:291: self.ninja.build(self.name, 'phony', output) Hm, now I'm confused. Won't this ...
8 years, 11 months ago (2012-01-07 21:29:00 UTC) #6
Nico
http://codereview.chromium.org/9121011/diff/12001/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): http://codereview.chromium.org/9121011/diff/12001/pylib/gyp/generator/ninja.py#newcode291 pylib/gyp/generator/ninja.py:291: self.ninja.build(self.name, 'phony', output) On 2012/01/07 21:29:00, Evan Martin wrote: ...
8 years, 11 months ago (2012-01-07 21:32:33 UTC) #7
Evan Martin
LGTM
8 years, 11 months ago (2012-01-09 06:58:43 UTC) #8
Nico
7 years, 3 months ago (2013-09-03 17:51:52 UTC) #9
Message was sent while issue was closed.
https://codereview.chromium.org/9121011/diff/12001/test/mac/gyptest-sourceles...
File test/mac/gyptest-sourceless-module.gyp (right):

https://codereview.chromium.org/9121011/diff/12001/test/mac/gyptest-sourceles...
test/mac/gyptest-sourceless-module.gyp:1: #!/usr/bin/env python
Durr, this should've been a .py file, not a .gyp. This test hasn't been running
for the last 1.5 years. Not surprisingly, it's broken now.

Powered by Google App Engine
This is Rietveld 408576698