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

Multiple fixes for windows registry handling #264

Merged
merged 2 commits into from
Apr 6, 2016
Merged

Conversation

brson
Copy link
Contributor

@brson brson commented Apr 5, 2016

This includes @petrochenkov's fix for non-existant HKCU\Environment\PATH, as well as fixes for #261, wherein we were setting this key to be a REG_SZ type instead of REG_EXPAND_SZ.

My second commit adds tests for more scenarios brought up by @petrochenkov's handling of non-existing registry keys.

@brson
Copy link
Contributor Author

brson commented Apr 5, 2016

The problem in #263 was failure to null-terminate, which really messes things up!

@Diggsey
Copy link
Contributor

Diggsey commented Apr 5, 2016

This is still going to bust any PATH which contains invalid unicode, which is unfortunate. When I get time I'd like to implement the solution using OsString which I mentioned on the previous PR.

@bors
Copy link
Contributor

bors commented Apr 5, 2016

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

@brson
Copy link
Contributor Author

brson commented Apr 5, 2016

@brson
Copy link
Contributor Author

brson commented Apr 5, 2016

Yeah, blanking the regkey if it's non-unicode is bad. Should probably be fixed in this pr.

let environment = root.open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE);

if environment.is_err() { return }
let environment = environment.expect("");
Copy link
Member

Choose a reason for hiding this comment

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

Could this be:

let environment = match environment {
    Ok(e) => e,
    Err(..) => return,
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I don't like that it takes four lines to do an early return.

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'll change it anyway ... grumble

@alexcrichton
Copy link
Member

r=me with the unicode bits fixed up (that you mentioned)

@brson brson force-pushed the winreg branch 2 times, most recently from f69b97e to c478a59 Compare April 5, 2016 23:20
@brson
Copy link
Contributor Author

brson commented Apr 5, 2016

Updated for feedback.

Now when rustup encounters a non-unicode PATH on windows during install or uninstall it will print a warning and not attempt to modify it.

@brson
Copy link
Contributor Author

brson commented Apr 5, 2016

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Apr 5, 2016

📌 Commit c478a59 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Apr 5, 2016

⌛ Testing commit c478a59 with merge 36f916d...

bors added a commit that referenced this pull request Apr 5, 2016
Multiple fixes for windows registry handling

This includes @petrochenkov's fix for non-existant HKCU\Environment\PATH, as well as fixes for #261, wherein we were setting this key to be a `REG_SZ` type instead of `REG_EXPAND_SZ`.

My second commit adds tests for more scenarios brought up by @petrochenkov's handling of non-existing registry keys.
@bors
Copy link
Contributor

bors commented Apr 6, 2016

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

@alexcrichton
Copy link
Member

(I'd just merge this, I doubt bors will ever get around to finishing)

@bors
Copy link
Contributor

bors commented Apr 6, 2016

💥 Test timed out

petrochenkov and others added 2 commits April 6, 2016 17:32
Fixes rust-lang#261

Also throw in some compatibility code to automatically fix this
on machines we've broken.
@brson brson merged commit 27d54ae into rust-lang:master Apr 6, 2016
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.

None yet

5 participants