fix #78 #79
fix #78 #79
Conversation
|
@hdhoang Thanks for the PR! The change looks pretty straight-forward, but I'm wondering if this added functionality belongs in |
|
I have moved this functionality to a separate method, and add a note about it in the README |
src/common.rs
Outdated
| "LD_LIBRARY_PATH" | ||
| }; | ||
|
|
||
| let lib_paths = env::var(varname).unwrap_or_else(|e| { |
There was a problem hiding this comment.
Why inspect the PATH variable? Isn't it the paths that end up on self.target_rustcflags that should be inspected?
let mut cfg = compiletest::Config::default();
cfg.target_rustcflags = Some(" -L target/debug");
cfg.clean_rmeta();
Maybe clean_rmeta() actually needs to inspect cfg.build_base?
There was a problem hiding this comment.
Actually, just curious, would it work any differently if you used the tmp feature?
There was a problem hiding this comment.
test-project uses tmp, while the example hdhoang/testee in #78 doesn't, and they behave the same.
AIUI, from the test's perspective, the tested crate's rlib is a dependency, thus it will be placed in the workspace's target/{debug,release}/deps. cargo check (--release) also places its rmeta there, causing this conflict.
mgattozzi/github-rs#109 is an example where breaking compile-tests out into another workspace avoids this issue. diesel does the same, but in that case there are genuine rlib conflicts.
I couldn't see that directory anywhere in Config's members, so I resorted to crudely iterating over dylib_env_var. (sidenote: I will update this to use dylib_env_var after #80 merges)
There was a problem hiding this comment.
This target/{profile}/deps path is defined in cargo::ops::cargo_rustc::Layout, used by Context and isn't exposed by that struct. I think the most we can do is constructing it from the build profile. Should I switch over to that, if possible?
There was a problem hiding this comment.
Ah I see what you mean now. I will look for it in target_rustcflags.
src/common.rs
Outdated
| }; | ||
|
|
||
| let lib_paths = env::var(varname).unwrap_or_else(|e| { | ||
| panic!( |
There was a problem hiding this comment.
Could this be one line? I really don't like how rustfmt splits this on five lines...
There was a problem hiding this comment.
I will restore link_deps's formatting.
|
I have reworked |
|
@laumann Could you please take a look at this PR? |
| @@ -233,6 +234,26 @@ impl Config { | |||
| self.target_rustcflags = Some(flags); | |||
| } | |||
|
|
|||
| /// Remove rmeta files from target `deps` directory | |||
There was a problem hiding this comment.
I'd like a little more documentation here, to explain why this is useful. IIRC this feature is useful when working with cargo check vs cargo build because the two methods don't share any information.
|
@hdhoang LGTM modulo a little more documentation of |
This method removes */deps/*.rmeta files in directories in rustcflags. These files are created by `cargo check`, and conflict with `cargo build` rlib files, causing E0464 for tests which use the parent crate. Fix #78
|
I have worked the commit message into the doc comment. Please have a look. |
|
Did you just push this or did I completely miss it? Either way it looks good |
|
Sorry, I often force push PRs. GitHub doesn't push new notifications for that event so I follow up with a comment. |
|
That's alright, I was just wondering if I overlooked something again. It looks good to me, I'm ready to merge if you don't have any further changes. |
|
I feel this is good to go now. |
These paths come from the rustc build environment, so I suppose cleaning their
*.rmetafiles here is appropriate.I'm ignoring errors here because: