Skip to content
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.

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

test: Move run-make tests into compiletest #33093

Merged
merged 1 commit into from
Apr 29, 2016

Conversation

alexcrichton
Copy link
Member

Forcing them to be embedded in makefiles precludes being able to run them in
rustbuild, and adding them to compiletest gives us a great way to leverage
future enhancements to our "all encompassing test suite runner" as well as just
moving more things into Rust.

All tests are still Makefile-based in the sense that they rely on make being
available to run them, but there's no longer any Makefile-trickery to run them
and rustbuild can now run them out of the box as well.

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@alexcrichton
Copy link
Member Author

r? @nikomatsakis

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 19, 2016

📌 Commit 9b9c498 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Apr 22, 2016

⌛ Testing commit 9b9c498 with merge 8e6113a...

@bors
Copy link
Contributor

bors commented Apr 22, 2016

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

@alexcrichton
Copy link
Member Author

@bors: retry

On Fri, Apr 22, 2016 at 8:27 AM, bors [email protected] wrote:

[image: 💔] Test failed - auto-win-msvc-64-opt-mir
http://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt-mir/builds/404


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#33093 (comment)

@bors
Copy link
Contributor

bors commented Apr 23, 2016

☔ The latest upstream changes (presumably #33020) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Apr 24, 2016

🔒 Merge conflict

@alexcrichton
Copy link
Member Author

@bors: r=nikomatsakis 3bc36dc

@bors
Copy link
Contributor

bors commented Apr 24, 2016

⌛ Testing commit 3bc36dc with merge f791063...

@bors
Copy link
Contributor

bors commented Apr 24, 2016

💔 Test failed - auto-linux-64-cross-netbsd

@alexcrichton
Copy link
Member Author

@bors: r=nikomatsakis 3462117

@bors
Copy link
Contributor

bors commented Apr 25, 2016

⌛ Testing commit 3462117 with merge 22463a8...

@bors
Copy link
Contributor

bors commented Apr 25, 2016

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton
Copy link
Member Author

@bors: r=nikomatsakis 3ca5b15

@bors
Copy link
Contributor

bors commented Apr 25, 2016

⌛ Testing commit 3ca5b15 with merge b11a7c8...

@bors
Copy link
Contributor

bors commented Apr 27, 2016

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

@alexcrichton alexcrichton force-pushed the rustbuild-rmake branch 3 times, most recently from 93a4d10 to 9e1bf9f Compare April 27, 2016 16:27
@alexcrichton
Copy link
Member Author

@bors: r=nikomatsakis 9e1bf9f

@bors
Copy link
Contributor

bors commented Apr 27, 2016

⌛ Testing commit 9e1bf9f with merge bf67681...

@bors
Copy link
Contributor

bors commented Apr 27, 2016

💔 Test failed - auto-linux-32-nopt-t

@alexcrichton
Copy link
Member Author

@bors: r=nikomatsakis 4bbf7cb

@bors
Copy link
Contributor

bors commented Apr 28, 2016

⌛ Testing commit 4bbf7cb with merge 2f8c3f6...

@bors
Copy link
Contributor

bors commented Apr 28, 2016

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

Forcing them to be embedded in makefiles precludes being able to run them in
rustbuild, and adding them to compiletest gives us a great way to leverage
future enhancements to our "all encompassing test suite runner" as well as just
moving more things into Rust.

All tests are still Makefile-based in the sense that they rely on `make` being
available to run them, but there's no longer any Makefile-trickery to run them
and rustbuild can now run them out of the box as well.
@alexcrichton
Copy link
Member Author

@bors: r=nikomatsakis 126e09e

@bors
Copy link
Contributor

bors commented Apr 29, 2016

⌛ Testing commit 126e09e with merge c0c08e2...

bors added a commit that referenced this pull request Apr 29, 2016
test: Move run-make tests into compiletest

Forcing them to be embedded in makefiles precludes being able to run them in
rustbuild, and adding them to compiletest gives us a great way to leverage
future enhancements to our "all encompassing test suite runner" as well as just
moving more things into Rust.

All tests are still Makefile-based in the sense that they rely on `make` being
available to run them, but there's no longer any Makefile-trickery to run them
and rustbuild can now run them out of the box as well.
@bors bors merged commit 126e09e into rust-lang:master Apr 29, 2016
@alexcrichton alexcrichton deleted the rustbuild-rmake branch May 2, 2016 18:45
@pnkfelix
Copy link
Member

pnkfelix commented May 3, 2016

@alexcrichton why did you surround $(CC_$(1)) with single quotes, producing '$(CC_$(1))' in the makefile?

From what I can tell, this breaks my use of --enable-ccache (because it works by putting e.g. ccache clang in as the CC variable, but now those single quotes are causing the whole string "ccache clang" to be interpreted as the program name by the shell.

Is this to accommodate Windows or something?


Ah, the above is just noting the same thing as #33285

@alexcrichton
Copy link
Member Author

Yes the single quotes were to accomodate windows where cl.exe has lots of spaces in the path, currently.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request May 6, 2016
Looks like the real bug on nightlies is that the `llvm-pass` run-make test is
not actually getting the value of `LLVM_CXXFLAGS` correct. Namely, it's blank!
Now the only change rust-lang#33093 which actually affected this is that the argument
`$(LLVM_CXXFLAGS_$(2))` was moved up from a makefile rule into the definition of
a variable. Sounds innocuous?

Turns out the variable this was moved into is defined with `:=`, which means
that it's not recursively expanded, which basically means that it's expanded
immediately. Unfortunately part of this expansion involves running
`llvm-config`, which doesn't exist at the start of distcheck build!

This didn't show up on the bots because they run `make` *then* `make check`, and
the first step builds llvm-config so the next time `make` is loaded everything
is available. The distcheck bots, however, run just a plain `distcheck` so
`make` doesn't exist ahead of time. You can see this in action where the
distcheck bots start out with a bunch of "llvm-config not found" error messages.

This commit just changes a few variables to be defined with `=` which
essentially means they're lazily expanded. I did not run a full distcheck
locally, but this makes the initial "llvm-config not found" error messages go
away so I suspect that this is the fix.

Closes rust-lang#33379
bors added a commit that referenced this pull request May 6, 2016
mk: Try to fix nightlies again

Looks like the real bug on nightlies is that the `llvm-pass` run-make test is
not actually getting the value of `LLVM_CXXFLAGS` correct. Namely, it's blank!
Now the only change #33093 which actually affected this is that the argument
`$(LLVM_CXXFLAGS_$(2))` was moved up from a makefile rule into the definition of
a variable. Sounds innocuous?

Turns out the variable this was moved into is defined with `:=`, which means
that it's not recursively expanded, which basically means that it's expanded
immediately. Unfortunately part of this expansion involves running
`llvm-config`, which doesn't exist at the start of distcheck build!

This didn't show up on the bots because they run `make` *then* `make check`, and
the first step builds llvm-config so the next time `make` is loaded everything
is available. The distcheck bots, however, run just a plain `distcheck` so
`make` doesn't exist ahead of time. You can see this in action where the
distcheck bots start out with a bunch of "llvm-config not found" error messages.

This commit just changes a few variables to be defined with `=` which
essentially means they're lazily expanded. I did not run a full distcheck
locally, but this makes the initial "llvm-config not found" error messages go
away so I suspect that this is the fix.

Closes #33379 (hopefully)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants