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

Make the x tool use the x and x.ps1 scripts #105844

Merged
merged 1 commit into from
Dec 18, 2022

Conversation

albertlarsan68
Copy link
Member

This removes another python search from bootstrap.

r? @jyn514

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 17, 2022
src/tools/x/src/main.rs Outdated Show resolved Hide resolved
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Thanks! This looks roughly right, but please fix @Nilstrieb's comment. Also, there's a lot of cfgs - if you could have a single fn x_command() -> Command function that's different depending on cfg(windows), that would be easier to understand, I think.

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 17, 2022
@albertlarsan68
Copy link
Member Author

@rustbot ready

I applied the suggestions and corrected the comments.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 18, 2022
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Thanks! r=me with the last nit addressed and the comment on the first line changed to say "bootstrap" instead of "x.py"

src/tools/x/src/main.rs Outdated Show resolved Hide resolved
@albertlarsan68
Copy link
Member Author

@jyn514, I did what you asked. Do you want me to squash the commits?

@jyn514
Copy link
Member

jyn514 commented Dec 18, 2022

@jyn514, I did what you asked. Do you want me to squash the commits?

That would be great, thanks! If you can double check the new tool works for you on Windows that would also be helpful :) I can test on Linux.

"RemoteSigned",
"-Command",
])
.arg(dir.join("x.ps1"));
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this breaks for me when there's spaces in the file name:

PS C:\Users\Joshua Nelson\src\rust> x
C:\Users\Joshua : The term 'C:\Users\Joshua' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
At line:1 char:1
+ C:\Users\Joshua Nelson\src\rust\x.ps1
+ ~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (C:\Users\Joshua:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CommandNotFoundException

Maybe use .current_dir(dir).arg("./x.ps1") instead so we don't have to worry about quoting it properly?

src/tools/x/src/main.rs Outdated Show resolved Hide resolved
This removes another python search from bootstrap.
@albertlarsan68
Copy link
Member Author

I rebased, squashed and applied your suggestions. It works on Windows, but I cannot test on Linux.

@jyn514
Copy link
Member

jyn514 commented Dec 18, 2022

Works great! Thank you :)

@bors r+ rollup (the x tool isn't tested in CI, other than rustfmt)

@bors
Copy link
Contributor

bors commented Dec 18, 2022

📌 Commit 8348e05 has been approved by jyn514

It is now in the queue for this repository.

@bors bors 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 Dec 18, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2022
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#96584 (Fix `x setup -h -v` should work)
 - rust-lang#105420 (Remove dead code after destination propagation)
 - rust-lang#105844 (Make the x tool use the x and x.ps1 scripts)
 - rust-lang#105854 (remove redundant clone)
 - rust-lang#105858 (Another `as_chunks` example)
 - rust-lang#105870 (avoid .into() conversion to identical types)
 - rust-lang#105875 (don't destuct references just to reborrow)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c16b969 into rust-lang:master Dec 18, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 18, 2022
@albertlarsan68 albertlarsan68 deleted the x-rewrite branch December 19, 2022 12:35
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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants