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: don't try to install rls if ToolState is not Testing #45588

Merged
merged 1 commit into from
Nov 7, 2017

Conversation

Keruspe
Copy link
Contributor

@Keruspe Keruspe commented Oct 28, 2017

We already do that for the Dist Step so we would end up trying to install something that we didn't dist.

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 28, 2017
@shepmaster
Copy link
Member

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned aturon Nov 3, 2017
@shepmaster shepmaster added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Nov 3, 2017
@alexcrichton
Copy link
Member

Thanks! Could this use the return value of ensure to dispatch though?

@Keruspe
Copy link
Contributor Author

Keruspe commented Nov 4, 2017

Sure, I just naively did the same as in the Dist phase.

The Dist Step is not ran in that case so we would end up trying to
install something that we didn't dist.

Signed-off-by: Marc-Antoine Perennou <[email protected]>
@Keruspe
Copy link
Contributor Author

Keruspe commented Nov 4, 2017

I'm wondering, should we just split the block in two macro parameters, the first one being the dist step we depend on and the second one being the actual install_foo call?
This way we could add the check for the return value of ensure in the common boilerplate and it would be automatically handled once other tools listed in toolstate can be installed.

edit: looks like ensure doesn't return the same type for all the steps (PathBuf VS Option<PathBuf>), that's a bummer

@Keruspe
Copy link
Contributor Author

Keruspe commented Nov 6, 2017

@alexcrichton FYI, in case I made it unclear, I amended my initial commit with your suggestion

@alexcrichton
Copy link
Member

@bors: r+

Thanks!

@bors
Copy link
Contributor

bors commented Nov 6, 2017

📌 Commit 784528b has been approved by alexcrichton

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2017
@bors
Copy link
Contributor

bors commented Nov 7, 2017

⌛ Testing commit 784528b with merge 31040922a2f540ccc5293596559b4f4ae5eeae8a...

kennytm added a commit to kennytm/rust that referenced this pull request Nov 7, 2017
rustbuild: don't try to install rls if ToolState is not Testing

We already do that for the Dist Step so we would end up trying to install something that we didn't dist.
@kennytm
Copy link
Member

kennytm commented Nov 7, 2017

@bors retry — prioritizing rollup

kennytm added a commit to kennytm/rust that referenced this pull request Nov 7, 2017
rustbuild: don't try to install rls if ToolState is not Testing

We already do that for the Dist Step so we would end up trying to install something that we didn't dist.
bors added a commit that referenced this pull request Nov 7, 2017
Rollup of 9 pull requests

- Successful merges: #45470, #45588, #45682, #45714, #45751, #45764, #45778, #45782, #45784
- Failed merges:
@bors bors merged commit 784528b into rust-lang:master Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants