Skip to content

Check for correct env keys in Command #39338

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Check for correct env keys in Command #39338

wants to merge 3 commits into from

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Jan 27, 2017

Return an error if the key contains an ASCII equals sign (=) at the
non-first position.

@rust-highfive
Copy link
Contributor

r? @sfackler

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

@Kimundi Kimundi added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 27, 2017
@frewsxcv
Copy link
Member

Tests?

@alexcrichton
Copy link
Member

Looks good to me, can you be sure to squash the commits as well?

@tbu- tbu- force-pushed the pr_check_valid_env branch from bf6ac6b to ab677d8 Compare February 4, 2017 22:46
@tbu-
Copy link
Contributor Author

tbu- commented Feb 4, 2017

Squashed and rebased.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Feb 4, 2017

📌 Commit ab677d8 has been approved by alexcrichton

@frewsxcv frewsxcv mentioned this pull request Feb 5, 2017
Closed
@frewsxcv
Copy link
Member

frewsxcv commented Feb 5, 2017

@bors r-

This fails on Windows

https://ci.appveyor.com/project/rust-lang/rust/build/1.0.1822/job/47x30lo5vajnd8pf

@alexcrichton
Copy link
Member

ping @tbu-, want to help fix the tests?

tbu-
tbu- commented Feb 15, 2017
View reviewed changes
src/libstd/sys/windows/process.rs Outdated
@@ -35,12 +35,6 @@ use sys_common::{AsInner, FromInner};
// Command
////////////////////////////////////////////////////////////////////////////////

fn mk_key(s: &OsStr) -> OsString {
FromInner::from_inner(sys::os_str::Buf {
inner: s.as_inner().inner.to_ascii_uppercase()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea why this is here. It ASCII-uppercases all env keys, for no particular reason as far as I can tell -- It was introduced with the stabilisation of the process module, it wasn't there before (if my git is good enough).

Copy link
Member

Choose a reason for hiding this comment

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

Can the existing behavior be preserved? We can discuss the specifics in a further issue if you'd like to make a case for removal.

AFAIK Windows has case insensitive keys, so we canonicalize to uppercase.

@tbu- tbu- force-pushed the pr_check_valid_env branch from a88104d to edf4e36 Compare February 15, 2017 17:56
@tbu-
Check for correct env keys in Command
742089d 
Return an error if the key contains an ASCII equals sign (`=`) at the
non-first position.
@tbu- tbu- force-pushed the pr_check_valid_env branch from edf4e36 to 7621677 Compare February 15, 2017 18:03
@tbu-
Copy link
Contributor Author

tbu- commented Feb 15, 2017

Rebased and addressed comment.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Feb 15, 2017

📌 Commit 7621677 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Feb 15, 2017

⌛ Testing commit 7621677 with merge a56bac8...

@bors
Copy link
Collaborator

bors commented Feb 15, 2017

💔 Test failed - status-appveyor

@tbu- tbu- force-pushed the pr_check_valid_env branch from 7621677 to 082a6d0 Compare February 15, 2017 23:47
@tbu-
Copy link
Contributor Author

tbu- commented Feb 15, 2017

Removed that semicolon.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Feb 16, 2017

📌 Commit 082a6d0 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Feb 16, 2017

⌛ Testing commit 082a6d0 with merge b0c79ab...

@bors
Copy link
Collaborator

bors commented Feb 16, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Collaborator

bors commented Feb 17, 2017

⌛ Testing commit a96956b with merge 8a84892...

@bors
Copy link
Collaborator

bors commented Feb 17, 2017

💔 Test failed - status-appveyor

@tbu- tbu- force-pushed the pr_check_valid_env branch from a96956b to 4dfc6fd Compare February 17, 2017 13:56
@tbu-
Copy link
Contributor Author

tbu- commented Feb 17, 2017

Fixed variable name.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Feb 17, 2017

📌 Commit 4dfc6fd has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Feb 17, 2017

⌛ Testing commit 4dfc6fd with merge 435492e...

@bors
Copy link
Collaborator

bors commented Feb 17, 2017

💔 Test failed - status-appveyor

@codyps
Copy link
Contributor

codyps commented Feb 17, 2017

Why is the = accepted in the first position? Digging around for documentation in posix and windows hasn't turned anything up.

Also may make sense to document this restriction on Command::env.

@tbu- tbu- force-pushed the pr_check_valid_env branch from 4dfc6fd to 314649d Compare February 17, 2017 19:57
@tbu-
Copy link
Contributor Author

tbu- commented Feb 17, 2017

Made the borrow checker happy.

@jmesmon I don't know, but it is accepted on both Windows and Linux (it's an oddity on both). This is a restriction of the underlying platform, it's the same for env::var, env::set_var etc. But yea, one could document it...

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Feb 17, 2017

📌 Commit 314649d has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Feb 18, 2017

⌛ Testing commit 314649d with merge aa35101...

@bors
Copy link
Collaborator

bors commented Feb 18, 2017

💔 Test failed - status-travis

@tbu-
Copy link
Contributor Author

tbu- commented Feb 22, 2017

I don't know how that particular test failed. :) It doesn't do anything with the environment.

@alexcrichton
Copy link
Member

The error message was added in this PR though, so it looks like this PR did indeed cause that error?

@tbu-
Copy link
Contributor Author

tbu- commented Mar 7, 2017

I added some println!-debugging so I can see why it fails on Android. Can we test this without a chance of accidently getting merged?

@alexcrichton
Copy link
Member

If you've got docker locally you can test this out via ./src/ci/docker/run.sh android, but otherwise instead of sending to bors you can also just add ALLOW_PR=1 to the travis configuration for this builder. That'll build it on this PR but we should just be sure to remove that before the PR lands to not let it leak into master.

@alexcrichton
Copy link
Member

@tbu- any update on this PR to debug android?

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with android tests fixed!

@dylni dylni mentioned this pull request Sep 18, 2022
Open
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

Assignees

@sfackler sfackler

Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@tbu- @rust-highfive @frewsxcv @alexcrichton @bors @codyps @sfackler @Kimundi