Skip to content

Fix passing CC to maketest.py when CC has arguments. #27938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix passing CC to maketest.py when CC has arguments. #27938

wants to merge 2 commits into from

Conversation

@DiamondLovesYou
Copy link
Contributor

@DiamondLovesYou DiamondLovesYou commented Aug 22, 2015

For example, I configure with CC="ccache /usr/lib/icecc/bin/gcc".

@DiamondLovesYou
Fix passing CC to maketest.py when CC has arguments.
c443b27 
For example, I configure with CC="ccache /usr/lib/icecc/bin/gcc".
@rust-highfive
Copy link
Contributor

rust-highfive commented Aug 22, 2015

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

alexcrichton commented Aug 22, 2015

@bors: r+ c443b27

@bors
Copy link
Collaborator

bors commented Aug 23, 2015

⌛ Testing commit c443b27 with merge 5022263...

@bors
Copy link
Collaborator

bors commented Aug 23, 2015

💔 Test failed - auto-win-msvc-64-opt

@DiamondLovesYou
Copy link
Contributor Author

DiamondLovesYou commented Aug 24, 2015

Fixed.

@alexcrichton
Copy link
Member

alexcrichton commented Aug 24, 2015

@bors: r+ 8ac323f589d9c1c346b0945868d675b4fb540c29

@bors
Copy link
Collaborator

bors commented Aug 24, 2015

⌛ Testing commit 8ac323f with merge f9fba9b...

@bors
Copy link
Collaborator

bors commented Aug 24, 2015

💔 Test failed - auto-win-msvc-64-opt

@DiamondLovesYou
Copy link
Contributor Author

DiamondLovesYou commented Aug 24, 2015

I could be wrong, but I think the windows builds have been silently ignoring the run-make tests because the filter 'option' has been filled with the rustdoc bin.

@DiamondLovesYou
Copy link
Contributor Author

DiamondLovesYou commented Aug 24, 2015

Also, on hosts using the MSVC compiler, run-pass tests that use $(CC) really can't be run from inside msys, due to the path sandboxing that goes on. I'm adding logic to those tests so they're skipped.

Edit: grammars.

@alexcrichton
Copy link
Member

alexcrichton commented Aug 24, 2015

Can you open an issue for the ignored tests on MSVC? It's fine to point them all to the same issue.

@DiamondLovesYou
Copy link
Contributor Author

DiamondLovesYou commented Aug 24, 2015

@alexcrichton Done

@alexcrichton
Copy link
Member

alexcrichton commented Aug 24, 2015

@bors: r+ 09210f0b6e12c7459e4857e5c654049d1756ef73

@bors
Copy link
Collaborator

bors commented Aug 24, 2015

⌛ Testing commit 09210f0 with merge 5ec686d...

@bors
Copy link
Collaborator

bors commented Aug 24, 2015

💔 Test failed - auto-win-msvc-64-opt

@DiamondLovesYou
Copy link
Contributor Author

DiamondLovesYou commented Aug 24, 2015

@alexcrichton I believe I got the rest of the tests referencing $(CC).

@alexcrichton
Copy link
Member

alexcrichton commented Aug 25, 2015

@bors: r+ 5e322bf

@bors
Copy link
Collaborator

bors commented Aug 25, 2015

⌛ Testing commit 5e322bf with merge c9983e6...

bors added a commit that referenced this pull request Aug 25, 2015
@bors
Auto merge of #27938 - DiamondLovesYou:run-make-cc-arg, r=alexcrichton
c9983e6 
For example, I configure with CC="ccache /usr/lib/icecc/bin/gcc".
@DiamondLovesYou
Copy link
Contributor Author

DiamondLovesYou commented Aug 26, 2015

Fixed.

@DiamondLovesYou DiamondLovesYou mentioned this pull request Aug 26, 2015
Closed
@alexcrichton
Copy link
Member

alexcrichton commented Aug 27, 2015

Ok, I was a little uncomfortable with the level of spray of "if MSVC don't do this" and at this point it's just easier to fix everything. I've tested alexcrichton@ca1c2a3 and it passes for me locally on MSVC, and I believe it shouldn't affect other platforms so can you include that into this PR?

@DiamondLovesYou DiamondLovesYou force-pushed the run-make-cc-arg branch 2 times, most recently from 4ea628e to 901412e Compare August 30, 2015 22:18
@DiamondLovesYou
Copy link
Contributor Author

DiamondLovesYou commented Aug 31, 2015

That's great, thanks (I didn't really have the time to figure this stuff out myself)! I've merged your changes.

alexcrichton
alexcrichton reviewed Aug 31, 2015
View reviewed changes
src/test/run-make/compiler-lookup-paths/Makefile Outdated
Copy link
Member

@alexcrichton alexcrichton Aug 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm I think I fixed this one on my branch? Perhaps a bad merge?

@DiamondLovesYou
Fix passing CC to maketest.py when CC has arguments.
43cc6d4 
rmake: Get all tests passing on MSVC (thanks Alex!).

For example, I configure with CC="ccache /usr/lib/icecc/bin/gcc".
@DiamondLovesYou
Copy link
Contributor Author

DiamondLovesYou commented Sep 1, 2015

Fixed.

@alexcrichton
Copy link
Member

alexcrichton commented Sep 1, 2015

@bors: r+ 43cc6d4

@bors
Copy link
Collaborator

bors commented Sep 1, 2015

⌛ Testing commit 43cc6d4 with merge 0a4ee56...

@bors
Copy link
Collaborator

bors commented Sep 1, 2015

💔 Test failed - auto-win-msvc-64-opt

alexcrichton
alexcrichton reviewed Sep 1, 2015
View reviewed changes
src/test/run-make/tools.mk
Copy link
Member

@alexcrichton alexcrichton Sep 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah it looks like this needs the same cygpath -w treatment as some above flags, wheeeee

@alexcrichton alexcrichton mentioned this pull request Sep 15, 2015
Merged
@alexcrichton
Copy link
Member

alexcrichton commented Sep 15, 2015

Continuing in #28421

bors added a commit that referenced this pull request Sep 17, 2015
@bors
Auto merge of #28421 - alexcrichton:msvc-rmake, r=alexcrichton
8ea2198 
Work carried over from #27938
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@alexcrichton alexcrichton

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@DiamondLovesYou @rust-highfive @alexcrichton @bors @nikomatsakis