-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Have compilation context info available earlier in the build process #5348
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
@bors r+ |
📌 Commit 84eb12f has been approved by |
⌛ Testing commit 84eb12f7d2e0aa30c40bb524d46690b2275aaab4 with merge 43308b5990ef0662dc0305eb80459de9ce93289b... |
💔 Test failed - status-travis |
84eb12f
to
3f3f27b
Compare
Ok(BuildConfig { | ||
host_triple: host_triple.to_string(), | ||
requested_target: (*requested_target).clone(), | ||
jobs: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pass in jobs: Option<u32>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I didn't do that is that passing None
explicitly always feels mostly ugly, especially in this situation where there are only two callers, and 1
seems pretty defensible as a default. All the fields of the struct are already being modified, anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(But if you insist, sure!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to argue that 1
is a completely wrong default here, and we should just use num_cpus
, but looks like we already use 1
in clean
, so I am actually fine with it now :-)
So that last one went a little bit crazy. It does seem like this is much clearer about the actual workings, though, in that |
This looks great to me, but let's also ask @alexcrichton about this refacrtoring! |
It also seems like RLS might need updating after this PR as well: they use cargo as a library, and so are pretty sensitive to internal organization. |
@bors: r+ |
📌 Commit a340ba0 has been approved by |
Have compilation context info available earlier in the build process Eventually, I hope this will allow us to ignore platform-specific dependencies when irrelevant for the current build earlier in the process. This should save on extraneous errors. As is, this seems like it already decreases coupling in the code base.
☀️ Test successful - status-appveyor, status-travis |
Eventually, I hope this will allow us to ignore platform-specific dependencies when irrelevant for the current build earlier in the process. This should save on extraneous errors. As is, this seems like it already decreases coupling in the code base.