Chromium Code Reviews
Help | Chromium Project | Sign in
(228)

Issue 11186038: Don't try to use -MMD / .o.d depfiles for ninja with .s files. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by Ami Fischman
Modified:
1 year, 6 months ago
CC:
gyp-developer_googlegroups.com
Base URL:
http://git.chromium.org/external/gyp.git@master
Visibility:
Public.

Description

Don't try to use -MMD / .o.d depfiles for ninja with .s files.

BUG=gyp:297
r1521

Patch Set 1 : . #

Patch Set 2 : with test #

Patch Set 3 : Create empty.s instead of relying on tools to be able to deal with empty files #

Patch Set 4 : Give win32/64 a free pass. #

Total comments: 6

Patch Set 5 : Clarify comment and include explicit empty.s file. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+54 lines, -3 lines) Lint Patch
M pylib/gyp/generator/ninja.py View 2 chunks +8 lines, -1 line 0 comments ? errors Download
A test/ninja/s-needs-no-depfiles/empty.s View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments ? errors Download
A test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py View 1 2 3 4 1 chunk +42 lines, -0 lines 0 comments ? errors Download
A + test/ninja/s-needs-no-depfiles/s-needs-no-depfiles.gyp View 1 1 chunk +3 lines, -2 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 8
Ami Fischman
1 year, 6 months ago #1
Nico (ooo Apr 18 - Apr 20)
Needs test. Does Evan's suggestion work too?
1 year, 6 months ago #2
Ami Fischman
On 2012/10/18 00:32:12, Nico wrote: > Needs test. Suggestions of copy/pasta source? > Does Evan's ...
1 year, 6 months ago #3
Ami Fischman
Added a test. Trybots are all grey on rietveld (launched with: git cl try). Can ...
1 year, 6 months ago #4
Ami Fischman
finally have all-green bot runs.
1 year, 6 months ago #5
Nico (ooo Apr 18 - Apr 20)
lgtm https://codereview.chromium.org/11186038/diff/2005/test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py File test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py (right): https://codereview.chromium.org/11186038/diff/2005/test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py#newcode9 test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py:9: were used for them (since clang & gcc ...
1 year, 6 months ago #6
Ami Fischman
https://codereview.chromium.org/11186038/diff/2005/test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py File test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py (right): https://codereview.chromium.org/11186038/diff/2005/test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py#newcode9 test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py:9: were used for them (since clang & gcc ignore ...
1 year, 6 months ago #7
Nico (ooo Apr 18 - Apr 20)
1 year, 6 months ago #8
(still lgtm)

https://codereview.chromium.org/11186038/diff/2005/test/ninja/s-needs-no-depf...
File test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py (right):

https://codereview.chromium.org/11186038/diff/2005/test/ninja/s-needs-no-depf...
test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py:9: were used for
them (since clang & gcc ignore -MMD when building .s->.o).
On 2012/10/18 18:14:38, Ami Fischman wrote:
> On 2012/10/18 18:09:39, Nico wrote:
> > "when building .s->o on linux" (doesn't happen on os x)
> 
> Are you sure?

Pretty sure. See the ninja-build thread on this, where I checked.

> (does this test pass for you on osx without the corresponding change to
> ninja.py?)

Yes:

hummer:gyp-rw thakis$ python gyptest.py
test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py 
PYTHONPATH=/Volumes/MacintoshHD2/src/gyp-rw/test/lib
TESTGYP_FORMAT=make
/usr/bin/python test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py
Invalid test for 'make' format; skipping test.
NO RESULT
TESTGYP_FORMAT=ninja
/usr/bin/python test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py
PASSED
TESTGYP_FORMAT=xcode
/usr/bin/python test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py
Invalid test for 'xcode' format; skipping test.
NO RESULT

No result from the following 2 tests:
	test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py
	test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py
hummer:gyp-rw thakis$ svn revert pylib/gyp/generator/ninja.py
Reverted 'pylib/gyp/generator/ninja.py'
^P^Phummer:gyp-rw thakpython gyptest.py
test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py -f ninja
PYTHONPATH=/Volumes/MacintoshHD2/src/gyp-rw/test/lib
TESTGYP_FORMAT=ninja
/usr/bin/python test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py
PASSED
hummer:gyp-rw thakis$

https://codereview.chromium.org/11186038/diff/2005/test/ninja/s-needs-no-depf...
test/ninja/s-needs-no-depfiles/gyptest-s-needs-no-depfiles.py:30:
test.write(os.path.join(test.workdir, 'empty.s'), '')
On 2012/10/18 18:14:38, Ami Fischman wrote:
> On 2012/10/18 18:09:39, Nico wrote:
> > Can't you just check in an empty 'empty.s'?
> 
> I did that with an earlier revision, but it made git try cry b/c one of the
> patching steps apparently can't cope with empty files...

"# This file intentionally left empty." Regular bots can handle empty files too
I think, so maybe that's not even necessary.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6