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

.cargo/config relative paths in rustflags #7433

Closed
joshlf opened this issue Sep 25, 2019 · 10 comments
Closed

.cargo/config relative paths in rustflags #7433

joshlf opened this issue Sep 25, 2019 · 10 comments
Labels
A-configuration Area: cargo config files and env vars A-rustflags Area: rustflags C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`

Comments

@joshlf
Copy link

joshlf commented Sep 25, 2019

Describe the problem you are trying to solve

I'd like to be able to set the rustflags key to a path which is relative to the location of the .cargo/config file (e.g., rustflags = ["-L", "../../out/libs"]) and have it work regardless of where cargo is invoked from.

Describe the solution you'd like

I could see multiple possible solutions:

  • Introduce a new key whose value is passed as the argument to -L to the compiler, and whose argument is interpreted as a relative path just like other key arguments already are
  • Treat any string of the form ./xxx or ../xxx in rustflags as a relative path, and modify it as appropriate

Maybe there's a better, third option though.

@joshlf joshlf added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Sep 25, 2019
@ratmice
Copy link

ratmice commented Sep 25, 2019

I do not believe this will really work even if .cargo/config rustflags is modified to know about relative paths. In the cargo configuration example here
it shows that .cargo/config is looked up throughout the parent directories.

however, I believe if baz/ depends on a crate
quux = { path = "../quux" }

quux/.cargo/config is not consulted meaning that if quux/.cargo/config specifies a relative path
compiling quux itself will specify the -L flags, but compiling quux from baz will not.
It seems like this would be enabling a very fragile use case.

@joshlf
Copy link
Author

joshlf commented Sep 26, 2019

The way I've seen .cargo/config used (and the way we use it on my team) is for workspace-wide configurations. In our case, our source code setup looks something like:

src/
    .cargo/
           config
    foo/
    bar/

So it applies when compiling any crate under src, including foo and bar.

@ratmice
Copy link

ratmice commented Sep 26, 2019

The potential fragility i'm concerned about, is if we were to try and embed your team's repository in a git submodule,

If I understand, the .cargo/config will work fine, when building from your workspace at top-level.
But, when brought into a different top-level it will start failing to apply the .cargo/config and then fail to link due to the missing the -L ../../out/libs

I'm not exactly familiar with a case which requires one to pass -L in this way, so i'm struggling a bit to figure out how to produce an example which shows the option working for team joshlf but failing for team rustafarians, so i'm not exactly sure my concern here is truly justifiable.

@ehuss ehuss added the A-configuration Area: cargo config files and env vars label Oct 10, 2019
@joshlf
Copy link
Author

joshlf commented Oct 17, 2019

Sorry for the late reply.

Concretely, our (Fuchsia's) build system has a single global directory into which all build artifacts are put. So what we'd like to do here is be able to have our .cargo/config file specify that directory specifically when you're in the context of our build. In other words, it's a property of your environment, not a property of any given dependency. It's orthogonal to the sorts of things that Cargo.toml is used for in that sense - Cargo.toml is about the crate itself, while .cargo/config is about the environment in which you're executing cargo.

However, I'd like to offer a simpler justification: This feature wouldn't change the behavior of the existing use case of absolute paths - it would only add a new use. If users of cargo found that they can't take advantage of relative paths for whatever reason, the fact that cargo supports them doesn't get in their way at all.

@ratmice
Copy link

ratmice commented Oct 17, 2019

joshlf: I think i see now, what confused me is that ../../out/libs/ is apparently outside of your build tree, and so if your rustc when called from your top-level is adding -L ../../../outlibs making the path relative to Fuchsia/foo when descending into it.

If I add Fuchsia as a git submodule to bar/Fuchsia, in order for your build to work i'd need to copy your Fuchsia/.cargo/config and mkdir -p ../out/libs/, If you need -L simply because you've also set '--out-dir', I have much less of an issue with it, I would just hate to see the balkanization of workspaces, where the workspace members only compile when being compiled from some specific top-level workspace.

@joshlf
Copy link
Author

joshlf commented Oct 18, 2019

Ah I see your confusion. Concretely, our tree is organized like this:

fuchsia/
        out
        src/
            .cargo/
                   config

The fuchsia/src/.cargo/config file would add -L ../../out, which is a relative path entirely contained within the fuchsia subtree pointing to fuchsia/out. The relative path doesn't escape the subtree, and so is valid regardless of where you put the fuchsia directory.

@ratmice
Copy link

ratmice commented Oct 18, 2019

My inclination be to attempt to ensure the path's don't escape .cargo/.. by calling
canonicalize and strip_prefix on them.

Unfortunately that doesn't appear to work for your case, as it doesn't appear that fuschia/ is a top-level Cargo really knows about (correct?), So my inclination goes exactly counter to your intended purpose.

Anyways what you say is true, nothing is stopping anyone from commiting hard coded paths already.

@joshlf
Copy link
Author

joshlf commented Oct 18, 2019

I think we'd be OK with the restriction that paths don't escape .cargo/.. - we could just add a new .cargo/config. That said, I'm not sure that buys us anything. In particular, relative paths are strictly more portable than absolute paths, and .cargo/config already uses absolute paths. In other words, while it's possible for somebody to introduce a relative path that would make the .cargo/config file non-portable, absolute paths are about as non-portable as you can get.

Given that, do you think it'd be reasonable to just allow the feature as originally proposed, with relative paths being evaluated relative to the .cargo/config file's location, and with no extra restrictions beyond that?

@ratmice
Copy link

ratmice commented Oct 18, 2019

@joshlf I suppose in the sense that it's perhaps too late to add a restriction on the paths.
If you happen to have a project where all cargo projects are at the same level you can already use an escaping relative path it seems, and this only makes it available to projects with a varying depth.

I believe this feature request, does not make an existing problem any worse. I think that's as diplomatic as I can be. So I suppose yes, it could be construed as reasonable.

@ehuss ehuss added the A-rustflags Area: rustflags label Apr 6, 2020
@epage
Copy link
Contributor

epage commented Nov 2, 2023

It looks like #12193 is a duplicate of this but as it has more discussion from the cargo team, I'm going to close this issue. If I missed something, let us know!

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-rustflags Area: rustflags C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`
Projects
None yet
Development

No branches or pull requests

4 participants