Skip to content

rustbuild: Install rustfmt as part of extended build #45980

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

Merged
merged 1 commit into from
Nov 17, 2017

Conversation

Keruspe
Copy link
Contributor

@Keruspe Keruspe commented Nov 14, 2017

Now that we distribute it, this allows ./x.py install to install it too

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 14, 2017
@Mark-Simulacrum
Copy link
Member

cc @aturon @nrc - are we ready for this?
r? @alexcrichton

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

This seems fine by me but we should be sure it works before it's added.

@@ -1401,6 +1405,10 @@ impl Step for Extended {
prepare("rls");
}

if rustfmt_installer.is_some() {
prepare("rustfmt");
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested this? I don't believe the combined installer is configured to have this work.

@@ -1444,6 +1452,9 @@ impl Step for Extended {
if rls_installer.is_some() {
prepare("rls");
}
if rustfmt_installer.is_some() {
prepare("rustfmt");
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested this? I don't think it will work as the combined installers aren't ready for this

(similar to other modifications below)

@nrc
Copy link
Member

nrc commented Nov 15, 2017

I'm not clear on the relationship between ./x.py install and the combined installers. The intention was that we would not be able to install rustfmt using the combined installers. So would that prevent this ever working?

@Keruspe
Copy link
Contributor Author

Keruspe commented Nov 15, 2017

Wasn't sure if this was ready either but figured it was worth a shot as the dist part has been merged.

It seems I indeed didn't test the combined installer (that's an oversight, as I tested it with ./x.py install). As it's not supposed to be installable using combined installers anyways, I just dropped that part.

I also have a patch to distribute cargo-fmt too, locally, if that's relevant @nrc .

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Nov 15, 2017

📌 Commit cc3a9ba 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 15, 2017
@bors
Copy link
Collaborator

bors commented Nov 16, 2017

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

@Keruspe
Copy link
Contributor Author

Keruspe commented Nov 16, 2017

Rebased and squashed

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Nov 16, 2017

📌 Commit 1930ee8 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Nov 17, 2017

⌛ Testing commit 1930ee8 with merge 8fbb46c...

bors added a commit that referenced this pull request Nov 17, 2017
rustbuild: Install rustfmt as part of extended build

Now that we distribute it, this allows `./x.py install` to install it too
@bors
Copy link
Collaborator

bors commented Nov 17, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 8fbb46c to master...

@bors bors merged commit 1930ee8 into rust-lang:master Nov 17, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants