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

parsing cargo:rustc-flags does not support the -L/libdir syntax (no spaces between flag and argument) #7217

Closed
filmil opened this issue Aug 5, 2019 · 1 comment
Labels
A-build-scripts Area: build.rs scripts C-bug Category: bug E-easy Experience: Easy

Comments

@filmil
Copy link

filmil commented Aug 5, 2019

Problem

Steps

  1. See: https://github.com/rust-lang/cargo/blob/master/src/cargo/core/compiler/custom_build.rs#L514
  2. Note that the parsing algorithm requires a space between the flag and the flag value.
  3. However, utilities like pkg-config will often print something like: -L/some/dir/local/lib -licui18n -licuuc -licudata, which is correct as far as flags go, but won't parse.

Possible Solution(s)

Perhaps the parsing should be corrected to account for this. And yes, I can replace -L with -L (space at end) and do the same for -l but would be nice if this worked out of the box.

Notes

Output of cargo version:

╰─>$ cargo version
cargo 1.37.0-nightly (4c1fa54d1 2019-06-24)
@filmil filmil added the C-bug Category: bug label Aug 5, 2019
@ehuss ehuss added A-build-scripts Area: build.rs scripts E-easy Experience: Easy labels Aug 5, 2019
@RobMor
Copy link
Contributor

RobMor commented Aug 17, 2019

Hi, I've gone ahead and taken a look at this. I believe I fixed the issue in my own fork and would like to write some tests for it. I'm brand new to cargo (and open source contributions in general) and don't really know where these tests would go. Right now I'm thinking tests/testsuite/build_script.rs would be a good place?

Also, judging by the other tests in that file, simply calling the parse_rustc_flags function in the usual unit-testing fashion is not what I should do?

RobMor added a commit to RobMor/cargo that referenced this issue Aug 17, 2019
bors added a commit that referenced this issue Aug 19, 2019
Fix #7217: Add support for rustc-flags without spaces between flags and values

Hi, I believe this pull request contains a fix for issue #7217. This is my first pull request to any open source project, much less any Rust project. I'm not super familiar with Rust at the moment, so let me know if I should change anything. Also any help/advice you can give me about this PR would be much appreciated!

I do have some specific questions:
- Is the test I added worthy of being its own test? I basically copied the one above it and remove the spaces between the flags and the values. Should I have added on to the test above it instead?
- Would it be better if I directly unit-tested this function in some other test file?
- Is the `ureachable!` macro good style?
@bors bors closed this as completed in fc94038 Aug 19, 2019
bgeron added a commit to bgeron/rust_icu that referenced this issue Jun 29, 2022
This removes a workaround for rust-lang/cargo#7217, which is no longer needed since Rust 1.40.0 and which made it impossible to have ICU installed in `~/my-libs/icu` (because of the `-l`).
kpozin pushed a commit to google/rust_icu that referenced this issue Jul 6, 2022
This removes a workaround for rust-lang/cargo#7217, which is no longer needed since Rust 1.40.0 and which made it impossible to have ICU installed in `~/my-libs/icu` (because of the `-l`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts C-bug Category: bug E-easy Experience: Easy
Projects
None yet
Development

No branches or pull requests

3 participants