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

Add space-friendly arguments #1509

Closed
wants to merge 2 commits into from
Closed

Add space-friendly arguments #1509

wants to merge 2 commits into from

Conversation

Zoxc
Copy link

@Zoxc Zoxc commented Feb 23, 2016

Add -C link-arg and -C llvm-arg which allows you to pass along argument with spaces.

Rendered

@nikomatsakis
Copy link
Contributor

@Zoxc

This seems like a well-motivated and reasonable RFC.

May I suggest however that you change this language from the drawbacks section:

We could leave rustc crippled and avoid having multiple arguments for passing arguments.

First, this is not a "drawback" of the current design (it sounds vaguely like an alternative). Second, the language is kind of judgemental and somewhat disrespectful in a variety of different ways. I don't think we could accept an RFC with language like this.

As for possible drawbacks and alternatives, here are some things I came up with off the top of my head:

  • Drawback: there would now be two "similar but different" options.
  • Alternative: one might consider deprecating the existing option in favor of the new one, to try and avoid the given drawback.

That said, I am ok with two similar but different options. :)

@comex
Copy link

comex commented Feb 27, 2016

Throwing out a possible alternative: change the original forms to do a basic shell parse on the arguments rather than splitting by spaces, so you could pass -C "link-args=foo 'bar baz'".

@Zoxc
Copy link
Author

Zoxc commented Feb 27, 2016

As @alexcrichton states in rust-lang/rust#30947 that is backwards incompatible. Although I doubt there is any practical impact. Shell escaping is also more complex than than a prefix.

@diwic
Copy link

diwic commented Mar 1, 2016

Related build failure on Ubuntu: rust-lang/rust#31529 - see a few comments down:

executing x86_64-unknown-linux-gnu/stage2/bin/rustc /«BUILDDIR»/rustc-1.8.0~~nightly.20160209+dfsg1/src/test/auxiliary/rustdoc-default-impl.rs -L x86_64-unknown-linux-gnu/test/rustdoc/ --target=x86_64-unknown-linux-gnu --crate-type=dylib -L x86_64-unknown-linux-gnu/test/rustdoc/default-impl.stage2-x86_64-unknown-linux-gnu.rustdoc.libaux -C prefer-dynamic --out-dir x86_64-unknown-linux-gnu/test/rustdoc/default-impl.stage2-x86_64-unknown-linux-gnu.rustdoc.libaux -C link-args='-Wl,-Bsymbolic-functions -Wl,-z,relro' --cfg rtopt -C rpath -O -L x86_64-unknown-linux-gnu/rt
error: unknown lint: `l,_z,relro'`

@tbelaire
Copy link

tbelaire commented Mar 1, 2016

Is it possible to add an example that passes two path to the linker?

# Alternatives
[alternatives]: #alternatives

None.
Copy link
Member

Choose a reason for hiding this comment

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

I think saying that there are no alternatives may be a bit disingenuous here. One possible alternative would be to invent some form of syntax which allows spaces to be escaped. A drawback of the alternative is that it is not strictly backwards compatible, but it is likely in practice compatible so that may not matter.

@alexcrichton alexcrichton added the T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. label Mar 7, 2016
@retep998
Copy link
Member

Having a way to pass linker arguments with spaces in them is a pretty essential requirement. I'm definitely in favor of this RFC.

@alexcrichton
Copy link
Member

🔔 This RFC is now entering its final comment period 🔔

The tools team discussed this RFC at triage yesterday and the conclusion was that this seems like some fine flags to add. @Zoxc would you be interested in updating with some of the feedback I left, however?

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jun 29, 2016
@brson
Copy link
Contributor

brson commented Jul 5, 2016

sgtm

@alexcrichton
Copy link
Member

The tools team got a chance to discuss this recently (sorry for the delay)! We felt that this was a minor enough addition to the compiler that we'd be fine just doing so as a PR. I'm gonna close this for now, but PRs are always welcome!

japaric pushed a commit to japaric/rust that referenced this pull request Sep 19, 2016
this flag lets you pass a _single_ argument to the linker but can be
used _repeatedly_. For example, instead of using:

```
rustc -C link-args='-l bar' (..)
```

you could write

```
rustc -C link-arg='-l' -C link-arg='bar' (..)
```

This new flag can be used with RUSTFLAGS where `-C link-args` has
problems with "nested" spaces:

```
RUSTFLAGS='-C link-args="-Tlayout.ld -nostartfiles"'
```

This passes three arguments to rustc: `-C` `link-args="-Tlayout.ld` and
`-nostartfiles"` to `rustc`. That's not what we meant. But this does
what we want:

```
RUSTFLAGS='-C link-arg=-Tlayout.ld -C link-arg=-nostartfiles`
```

cc rust-lang/rfcs#1509
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Sep 27, 2016
rustc: implement -C link-arg

this flag lets you pass a _single_ argument to the linker but can be
used _repeatedly_. For example, instead of using:

```
rustc -C link-args='-l bar' (..)
```

you could write

```
rustc -C link-arg='-l' -C link-arg='bar' (..)
```

This new flag can be used with RUSTFLAGS where `-C link-args` has
problems with "nested" spaces:

```
RUSTFLAGS='-C link-args="-Tlayout.ld -nostartfiles"'
```

This passes three arguments to rustc: `-C` `link-args="-Tlayout.ld` and
`-nostartfiles"` to `rustc`. That's not what we meant. But this does
what we want:

```
RUSTFLAGS='-C link-arg=-Tlayout.ld -C link-arg=-nostartfiles`
```

cc rust-lang/rfcs#1509

r? @alexcrichton
cc @Zoxc

This needs a test. Any suggestion?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-dev-tools Relevant to the development tools team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants