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

Display environment variables for rustc commands #6492

Merged
merged 2 commits into from Jan 3, 2019
Merged

Display environment variables for rustc commands #6492

merged 2 commits into from Jan 3, 2019

Conversation

ghost
Copy link

@ghost ghost commented Dec 27, 2018

This picks up on the work done in PR #5683.

The extra output is only displayed with -vv.

The Windows output has the form set FOO=foo && BAR=bar rustc ... instead of
the form that suggested in #5683 to make escaping easier and since it's
simpler.

This picks up on the work done in PR #5683.

The extra output is only displayed with `-vv`.

The Windows output has the form `set FOO=foo && BAR=bar rustc ...` instead of
the form that suggested in #5683 to make escaping easier and since it's
simpler.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 27, 2018

Real question, Does that form work? Opening CMD I get:

>set FOO=foo && BAR=bar rustc
'BAR' is not recognized as an internal or external command,
operable program or batch file.

@ehuss
Copy link
Contributor

ehuss commented Dec 28, 2018

@Eh2406 You need set before each variable.

This doesn't work very well on Windows. The escaping rules of cmd are finicky. For example, set FOO='bar' is embedding the single quotes within the variable (cmd doesn't use single quotes for strings). Also, it is space sensitive, so set FOO=one && BAR=two && rustc... will embed spaces after each variable (FOO is one ). I think avoiding the quotes, and just escaping special characters (like ^") might be an option. And avoid the space between commands (not sure if there's a way to avoid that).

Also, would it be possible to detect the shell and use different syntax? I suspect cmd users are rare (at least I don't know any). Most people I've seen use bash or powershell.

Remove the space at the end of the set commands.
@ghost
Copy link
Author

ghost commented Dec 28, 2018

@Eh2406

set FOO=foo && BAR=bar rustc

That was a mistake in my description, I acutally implemented set FOO=foo && set BAR=bar && rustc.

@ehuss

Also, it is space sensitive, so set FOO=one && BAR=two && rustc... will embed spaces after each variable (FOO is one ).

This should be fixed now.

I think avoiding the quotes, and just escaping special characters (like ^") might be an option.

So if I understand you, I should not shell escape the variables and only escape the cmd.exe metacharacters with ^. So "Ben & Jerry's" becomes ^"Ben ^& Jerry^'s^". This seemed to work when I tried it but I'm not sure about this. ProcessBuilder already escapes rustc arguments using shell_escape::escape and doesn't do this (although maybe it should). This article linked from the shell_escape docs seems to indicate that both types of escaping are required.

Also, would it be possible to detect the shell and use different syntax? I suspect cmd users are rare (at least I don't know any). Most people I've seen use bash or powershell.

This is out of scope for me. I have a feeling that the powershell escaping story might be even worse than cmd.exe.

@ehuss
Copy link
Contributor

ehuss commented Dec 29, 2018

This seemed to work when I tried it but I'm not sure about this.

I am not sure about it, either. However, from my understanding of cmd syntax, it works a bit differently from command-line parsing syntax. It seems to be more reliable to use ^ escaping. I think the argument escaping is probably already correct, since that goes through a different mechanism.

One thing I can't figure out is how to set an empty variable. set FOO= deletes the variable. I don't think cargo has any variables where that would matter, but it doesn't seem quite right.

This is out of scope for me. I have a feeling that the powershell escaping story might be even worse than cmd.exe.

That's fair.

@alexcrichton
Copy link
Member

This looks like a great start to me, thanks @mikerite! I agree that we'll probably want powershell syntax eventually, but for now I think it's fine to add this in and iterate from here

@bors: r

@bors
Copy link
Collaborator

bors commented Jan 3, 2019

📌 Commit e557f66 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Jan 3, 2019

⌛ Testing commit e557f66 with merge 34320d2...

bors added a commit that referenced this pull request Jan 3, 2019
Display environment variables for rustc commands

This picks up on the work done in PR #5683.

The extra output is only displayed with `-vv`.

The Windows output has the form `set FOO=foo && BAR=bar rustc ...` instead of
the form that suggested in #5683 to make escaping easier and since it's
simpler.
@bors
Copy link
Collaborator

bors commented Jan 3, 2019

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

@bors bors merged commit e557f66 into rust-lang:master Jan 3, 2019
bors added a commit to rust-lang/rust that referenced this pull request Jan 4, 2019
Update cargo

24 commits in 0d1f1bbeabd5b43a7f3ecfa16540af8e76d5efb4..34320d212dca8cd27d06ce93c16c6151f46fcf2e
2018-12-19 14:45:14  0000 to 2019-01-03 19:12:38  0000
- Display environment variables for rustc commands (rust-lang/cargo#6492)
- Fix a very minor race condition in `cargo fix`. (rust-lang/cargo#6515)
- Add a high-level overview of how `fix` works. (rust-lang/cargo#6516)
- Add dependency `registry` to `cargo metadata`. (rust-lang/cargo#6500)
- Fix fingerprint calculation for patched deps. (rust-lang/cargo#6493)
- serialize version directly (rust-lang/cargo#6512)
- use DYLD_FALLBACK_LIBRARY_PATH for dylib_path_envvar on macOS (rust-lang/cargo#6355)
- Fix error message when resolving dependencies (rust-lang/cargo#6510)
- use PathBuf in cargo metadata (rust-lang/cargo#6511)
- Fixed link to testsuite in CONTRIBUTING.md (rust-lang/cargo#6506)
- Update display of contents of Cargo.toml (rust-lang/cargo#6501)
- Update display of contents of Cargo.toml (rust-lang/cargo#6502)
- Fixup cargo install's help message (rust-lang/cargo#6495)
- testsuite: Require failing commands to check output. (rust-lang/cargo#6497)
- Delete unnecessary 'return' (rust-lang/cargo#6496)
- Fix new unused patch warning. (rust-lang/cargo#6494)
- Some minor documentation changes. (rust-lang/cargo#6481)
- Add `links` to `cargo metadata`. (rust-lang/cargo#6480)
- Salvaged semver work (rust-lang/cargo#6476)
- Warn on unused patches. (rust-lang/cargo#6470)
- don't write a an incorrect rustc version to the fingerprint file (rust-lang/cargo#6473)
- Rewrite `login` and registry cleanups. (rust-lang/cargo#6466)
- [issue#6461] Fix cargo commands list (rust-lang/cargo#6462)
- Restrict registry names to same style as package names. (rust-lang/cargo#6469)
@ehuss ehuss added this to the 1.33.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants