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

Rustc args: strings vs vectors & extracting link directories #149

Open
aldanor opened this issue Nov 19, 2018 · 4 comments
Open

Rustc args: strings vs vectors & extracting link directories #149

aldanor opened this issue Nov 19, 2018 · 4 comments

Comments

@aldanor
Copy link

aldanor commented Nov 19, 2018

@laumann

We need to parse the flags in utility methods like .clean_rmeta() which proves to be quite painful, e.g. link flags could be either -Lfoo or -L foo or -Lnative=foo, etc. The current method of splitting by whitespace is wrong and generally dangerous. It would also be hard to handle paths with spaces correctly.

One possible suggestion: instead, store flags as Vec<String>, so we can at least skip parsing quotes, whitespace, etc; each flags and its (optional) value to be stored in each element. An example would be ["-Lfoo", "-L", "foo", "-Lnative=foo"]. This would obviously be a breaking change.

Another option would be to still store flags as strings but only perform cleaning in the "default" locations (those added by the .link_deps()), but then the user folders would be ignored.

Finally, yet another option: still store flags as strings, but store link flags separately, link link_flags. It's also a little weird since then there would be an overlap.

@aldanor
Copy link
Author

aldanor commented Nov 19, 2018

Related: #148

@laumann
Copy link
Collaborator

laumann commented Nov 20, 2018

Yes, I agree, splitting by whitespace is dangerous and wrong, I had this discussion in #81.

We concluded at that time that what should ideally be changed is Config.target_rustcflags from Option<String> to Option<Vec<String>> (or Option<Vec<Path>> or something similar). This kind of change should be submitted for rustc's compiletest, because I'd prefer for this implementation and the original not to diverge too much.

But maybe we can sidestep the problem, by internally storing them somewhere else/outside of Config and just set Config.target_rustcflags when necessary? Would that make sense?

@laumann
Copy link
Collaborator

laumann commented Nov 20, 2018

See also #84, which wasn't merged.

@aldanor
Copy link
Author

aldanor commented Dec 11, 2018

Confirming that compiletest-rs doesn't really work on Windows (AppVeyor) due to paths like "C:\Program Files\What Ever\" that get wrongfully split by whitespace.

This is definitely broken and needs fixing (along with caching issues). I'll try another attempt at this, #84 and #148.

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

No branches or pull requests

2 participants