Fix: Don't parse the home directory more than once #3078
Conversation
This is the first version of the fix. It needs clean up.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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. |
|
|
||
| impl ConfigFile { | ||
| pub fn new(path: PathBuf) -> ConfigFile { | ||
| let canonical = match fs::canonicalize(path) { |
There was a problem hiding this comment.
I believe this didn't previously call fs::canonicalize, so perhaps this could be left out?
There was a problem hiding this comment.
No, it's new, but It was suggested in the original issue (here #3070 (comment)) and I thought it was a good idea. I can remove it of course.
There was a problem hiding this comment.
Now that I'm thinking, if I remove the fs::canonicalize call I don't really need the struct. I put it only to simplify the "canonicalization" process. I think it would be simpler if I remove the struct and leave only the HashSet.
|
Thanks @jhbabon! |
|
@alexcrichton any idea why the AppVeyor tests fail? I didn't touch those specs, I think. |
* Remove ConfiFile struct, it is not needed without the fs::canonicalize call. * Don't check if the file is in the stash when walking the tree, without "canonicalization" it is not necessary.
|
@alexcrichton I removed the call to |
Fix: Don't parse the home directory more than once This PR tries to resolve this issue #3070. The problem is that the `walk_tree` method in the `src/util/config.rs` module was parsing more than once the contents of the config file in the home directory (the file `~/.cargo/config`). The biggest problem with this is with options that can accept multiple values, like `build.rustflags`. If you parse the file twice, the same option can end with duplicated values (e.g: `rustflags=["-Z", "foo", "-Z", "foo"]`). I made the fix following the comments in the issue. In the fix I keep track of all the parsed config files in a `HashSet` so I can know if a file has been parsed already. ~~I'm also using `std::fs::canonicalize`, as suggested in the issue, to prevent parsing files behind symbolic links more than once.~~ **UPDATE:** I removed the call to `fs::canonicalize` as suggested in the comments. Now the fix is way simpler, which means less code and less possibilities to add a new bug.
|
💔 Test failed - cargo-win-gnu-32 |
|
@bors: retry On Fri, Sep 9, 2016 at 9:29 AM, bors notifications@github.com wrote:
|
|
⚡ Previous build results for cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64 are reusable. Rebuilding only cargo-cross-linux, cargo-win-gnu-32... |
|
☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64 |
|
Great! Thanks @alexcrichton ! |
This PR tries to resolve this issue #3070. The problem is that the
walk_treemethod in thesrc/util/config.rsmodule was parsing more than once the contents of the config file in the home directory (the file~/.cargo/config). The biggest problem with this is with options that can accept multiple values, likebuild.rustflags. If you parse the file twice, the same option can end with duplicated values (e.g:rustflags=["-Z", "foo", "-Z", "foo"]).I made the fix following the comments in the issue. In the fix I keep track of all the parsed config files in a
HashSetso I can know if a file has been parsed already.I'm also usingstd::fs::canonicalize, as suggested in the issue, to prevent parsing files behind symbolic links more than once.UPDATE: I removed the call to
fs::canonicalizeas suggested in the comments. Now the fix is way simpler, which means less code and less possibilities to add a new bug.