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

rustbuild: Automatically enable Ninja on MSVC #44084

Merged
merged 1 commit into from
Aug 26, 2017

Conversation

alexcrichton
Copy link
Member

Discovered in #43767 it turns out the default MSBuild generator in CMake for
whatever reason isn't supporting many of the configuration options we give to
LLVM. To improve the contributor experience automatically enable Ninja if we
find it to ensure that "flavorful" configurations of LLVM work by default in
more situations.

Closes #43767

Discovered in rust-lang#43767 it turns out the default MSBuild generator in CMake for
whatever reason isn't supporting many of the configuration options we give to
LLVM. To improve the contributor experience automatically enable Ninja if we
find it to ensure that "flavorful" configurations of LLVM work by default in
more situations.

Closes rust-lang#43767
@alexcrichton
Copy link
Member Author

r? @Mark-Simulacrum

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2017
// environment.
if !build.config.ninja && build.config.build.contains("msvc") {
if cmd_finder.maybe_have("ninja").is_some() {
build.config.ninja = true;
Copy link
Member

Choose a reason for hiding this comment

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

This means that there's no way to disable ninja on MSVC without editing bootstrap code. I'm not sure if there's any nice way of fixing that, though... or if it's even all that important to fix. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct, yes, but I'd consider it "if anyone runs into this then they have bigger problems" situations

Copy link
Member

Choose a reason for hiding this comment

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

True.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 25, 2017

📌 Commit e13f02e has been approved by Mark-Simulacrum

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Aug 26, 2017
…ulacrum

rustbuild: Automatically enable Ninja on MSVC

Discovered in rust-lang#43767 it turns out the default MSBuild generator in CMake for
whatever reason isn't supporting many of the configuration options we give to
LLVM. To improve the contributor experience automatically enable Ninja if we
find it to ensure that "flavorful" configurations of LLVM work by default in
more situations.

Closes rust-lang#43767
@bors
Copy link
Collaborator

bors commented Aug 26, 2017

⌛ Testing commit e13f02e with merge 003a929...

bors added a commit that referenced this pull request Aug 26, 2017
rustbuild: Automatically enable Ninja on MSVC

Discovered in #43767 it turns out the default MSBuild generator in CMake for
whatever reason isn't supporting many of the configuration options we give to
LLVM. To improve the contributor experience automatically enable Ninja if we
find it to ensure that "flavorful" configurations of LLVM work by default in
more situations.

Closes #43767
@bors
Copy link
Collaborator

bors commented Aug 26, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: Mark-Simulacrum
Pushing 003a929 to master...

@bors bors merged commit e13f02e into rust-lang:master Aug 26, 2017
@alexcrichton alexcrichton deleted the msvc-ninja branch August 27, 2017 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants