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 named path bases to cargo #3074

Closed
wants to merge 3 commits into from
Closed

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Feb 8, 2021

Introduce shared base directories in Cargo configuration files that in turn enable base-relative path dependencies.

Rendered.

Implementation PR.

This is the culmination of extended discussion on i.r-l.o.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 9, 2021

One neat suggestion I've gotten is to infer path from the package name if only base is specified. In other words, if you wrote:

foo = { base = "vendor" }

The path dependency would pull ${vendor}/foo. Not sure if we want something as implicit as that (it no longer says "path" anywhere), but it would be meaningful improvement in ergonomics. Thoughts?

@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Feb 9, 2021
@Aloso
Copy link

Aloso commented Feb 11, 2021

What's the rationale for defining the path bases in the cargo configuration rather than Cargo.toml?

I think that all necessary information for building a crate should be in a single directory that can be managed by git. This makes it easier to publish the code and for others to use it. What's the rationale for wanting to depend on crates in a completely different directory?

EDIT: I just read up on the discussion on IRLO, which provides more context for where this feature would be useful. I think this context should also be added to the RFC.

@Lokathor
Copy link
Contributor

because the actual base directory for "project_foo" is local to my machine, and it won't be the same as on your machine.

It's entirely reasonable for an individual git repo to say "from the project_foo base directory, go look here", but a random git repo on the internet cannot possibly know where I put that directory (because it's sure not /home/jon/dev/rust/, I don't even use unix).

@Aloso
Copy link

Aloso commented Feb 11, 2021

because the actual base directory for "project_foo" is local to my machine, and it won't be the same as on your machine.

I'd use either relative paths or git dependencies in that case.

The RFC currently describes a solution to a problem that I (as well as most Rust devs I believe) have never encountered. That's why I think the RFC needs further explanation why this is necessary.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 11, 2021

because the actual base directory for "project_foo" is local to my machine, and it won't be the same as on your machine.

I'd use either relative paths or git dependencies in that case.

Relative paths only really work if the target is also inside the same repository. Once you go beyond that, a path of ../foobar requires me to have the relevant package checked out into .. under the name foobar, which seems stricter than it should be. Relative paths also don't work well for paths that are build-dependent, such as the build-system generated paths mentioned in the motivation section of the RFC.

git dependencies aren't a tenable replacement, since they don't really work for the case where you want to make changes to the dependency and have them reflected when you build. They also don't cater to the use-cases in the motivation section, such as having a common path on your computer that you want a short-hand for, or a directory of dependencies that are automatically vendored by some surrounding build system.

The RFC currently describes a solution to a problem that I (as well as most Rust devs I believe) have never encountered. That's why I think the RFC needs further explanation why this is necessary.

I'm curious — does the motivation section not outline the intended use-cases sufficiently? What would you like to see added after reading the i.r-l.o thread?

@Aloso
Copy link

Aloso commented Feb 12, 2021

Relative paths only really work if the target is also inside the same repository

Well, that's why I usually put local dependencies in the same repository (and the same cargo workspace). If you want to use a local crate from multiple repositories, I believe you can use symlinks.

That doesn't cover the other use case with the external build system though.

I'm curious — does the motivation section not outline the intended use-cases sufficiently?

Before I read the IRLO thread, I didn't understand how the external build system should work or why it can't place the vendored deps into the crate being built. Also, I couldn't have read the RFC's motivation very carefully, because the second use-case slipped my mind when I was writing the first comment 😅

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 12, 2021

Relative paths only really work if the target is also inside the same repository

Well, that's why I usually put local dependencies in the same repository (and the same cargo workspace). If you want to use a local crate from multiple repositories, I believe you can use symlinks.

Ah, yes, the "symlink solution" should probably be discussed somewhere in the RFC. The short answer for why they're not ideal is that symlinks are very particular to UNIX (they're real hard to use on Windows) and that they end up polluting your directory listings. The former is (perhaps obviously) a bigger point than the latter 😅

I'm curious — does the motivation section not outline the intended use-cases sufficiently?

Before I read the IRLO thread, I didn't understand how the external build system should work or why it can't place the vendored deps into the crate being built. Also, I couldn't have read the RFC's motivation very carefully, because the second use-case slipped my mind when I was writing the first comment sweat_smile

All good — it happens to us all!

So the build system could place the vendored dependencies directly into the package, but that would mean that now if the dependencies change, you'd be forced to re-run the external build system, even though cargo itself has the mechanism to deal with that for path dependencies on its own.


I'll take a stab at updating the RFC with both of those alternative approaches tomorrow :)

@burdges
Copy link

burdges commented Feb 12, 2021

The path dependency would pull ${vendor}/foo. Not sure if we want something as implicit as that (it no longer says "path" anywhere), but it would be meaningful improvement in ergonomics. Thoughts?

I think cargo workspaces break this, but the form proposed here looks safe.

We'd love nested cargo workspaces but maybe this provides a reasonable middle ground, especially since git submodules cannot be symlinks.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 15, 2021

The path dependency would pull ${vendor}/foo. Not sure if we want something as implicit as that (it no longer says "path" anywhere), but it would be meaningful improvement in ergonomics. Thoughts?

I think cargo workspaces break this, but the form proposed here looks safe.

Hmm, break it in what way?

We'd love nested cargo workspaces but maybe this provides a reasonable middle ground, especially since git submodules cannot be symlinks.

Oh, yeah, I'd love nested workspaces. But I think realistically that's farther away, and I also think the proposal here adds some simpler mechanisms that may be useful for simpler cases than nested workspaces.

@burdges
Copy link

burdges commented Feb 15, 2021

Hmm, break it in what way?

It's purely that ${vender}/crate no longer makes sense with the intervening workspace, so similar to issues with nested cargo workspaces.

Afaik, the proposal written here works with cargo workspaces fine.

@joshtriplett
Copy link
Member

Oh, yeah, I'd love nested workspaces. But I think realistically that's farther away

If you have the bandwidth, this is one of our top "we want this but don't have time to do it" features. Want to work on it? :)

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 16, 2021

If you have the bandwidth, this is one of our top "we want this but don't have time to do it" features. Want to work on it? :)

Hehe 😅 I think they're probably overkill for what I'd use them for, and thus harder to motivate spending much time on. But if someone wrote up mentoring instructions for it, I might be able to take a look when time does allow. I tried looking around the nested workspaces issue, but it didn't seem like there was much in terms of consensus for how it should work or where someone might start — that would help a lot.

@pickfire
Copy link
Contributor

Is there a way not to have every developer to modify their cargo config?

What if the developer does not want to use a single directory for development, my source code is splattered around and sometimes I even use /tmp, is it possible to specify multiple paths for a single base or let it search recursively?

What is the case that one wants to use this but not cargo workspaces?

Comment on lines +99 to +107
The Cargo configuration files receive a new configuration table, `base`.
Each entry is a key-value pair where the key names a new path "base",
and the value specifies the path to that path base. The path can be
relative, in which case it is resolved relative to the containing
configuration file.

Dependency specifications gain a new field, `base`, which is expected to
hold the name of an already-defined path base. The field only carries
meaning for `path` dependencies (for now). To resolve a path dependency
Copy link

Choose a reason for hiding this comment

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

[bikeshed] Alternative name for both the configuration table and dependency field: path-base.

Arguments for:

  • More specific, arguably bit more self-documenting.
  • Associates the field clearly to the path, i.e. foo = { path = "foo", path-base = "dev" }
  • Clearer path for future git-base. Would resolve mentioned concern "However, this may get complicated if someone specifies git, path, and base."
    • I'm not aware of a use-case where sharing namespace for (git-) and (path-) base definitions would be useful.

Arguments against:

  • Longer.
  • Lower possibility to generalize the field/table in the future (could be both advantage and disadvantage).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know @joshtriplett had some thoughts around potentially using the same base keyword for both path and git, but if it's untenable then I wonder if we want them to diverge more. For example, we could have it be within or relative-to for path and keep base for URL-based dependencies (like git) since "base" is already a thing in HTML documents on the web.

Comment on lines +271 to +275
- Is there other reasonable behavior we could fall back to if a `base`
is specified for a dependency, but no base by that name exists in the
current Cargo configuration? This RFC suggests that this should be an
error, but perhaps there is a reasonable thing to try _first_ prior to
yielding an error.
Copy link

Choose a reason for hiding this comment

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

Given recent and less recent publicity of dependency confusion supply-chain vulnerabilities, making this a hard error (i.e. no fallbacks) seems to be the safest option.

Which would mean that this question could be considered as answered, and the RFC could potentially explicitly say that this would lead to error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree with that — I think explicit is better than implicit here.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 17, 2021

Is there a way not to have every developer to modify their cargo config?

What if the developer does not want to use a single directory for development, my source code is splattered around and sometimes I even use /tmp, is it possible to specify multiple paths for a single base or let it search recursively?

I'm not sure what you mean by "search recursively"?

One alternative here is to have named paths instead of named path bases, but that would mean you now need to declare one name for every path, and not just one for each collection of paths. I suppose we could support both?

What is the case that one wants to use this but not cargo workspaces?

You may have two crates that aren't published together. That is, even though A depends on B, it might not make sense to bundle A and B together — they may be hosted in different repositories, have different owners, or just otherwise be considered separate enough that linking them strongly doesn't seem like the right thing to do. The trivial example of this is if you have two different crates you're hacking on to make them work with some local patch to a third crate foo. You can't place foo in a workspace for both those two crates, so you need a path patch.

Also, nested workspaces aren't supported, which makes even the case where you could just place all the crates in one workspace pretty annoying.

@pickfire
Copy link
Contributor

pickfire commented Feb 18, 2021

You may have two crates that aren't published together. That is, even though A depends on B, it might not make sense to bundle A and B together — they may be hosted in different repositories, have different owners, or just otherwise be considered separate enough that linking them strongly doesn't seem like the right thing to do. The trivial example of this is if you have two different crates you're hacking on to make them work with some local patch to a third crate foo. You can't place foo in a workspace for both those two crates, so you need a path patch.

But can't one just use git and point to another private repository or since it is internal, they could talk about how they structure the project and they could use relative paths which will work as well, like path = '../b', a discussion on where to put stuff may be better than doing this extra indirection.

By the way, happy chinese new year!

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 19, 2021

But can't one just use git and point to another private repository or since it is internal, they could talk about how they structure the project and they could use relative paths which will work as well, like path = '../b', a discussion on where to put stuff may be better than doing this extra indirection.

I mean, you can always use a git dependency instead of a path dependency, but that doesn't really solve for the use-case where you are actively working on the dependency. Or where a build system wants to set up a common location for many such dependencies that it doesn't want the developer to have to hard-code the paths to.

By the way, happy chinese new year!

🎉

@sivadeilra
Copy link
Contributor

This RFC describes pretty much exactly what my company needs, in order to adopt Cargo at scale. How can we drive this forward?

@joshtriplett
Copy link
Member

We do think there may be value in a solution for this problem. However, the proposer (@jonhoo) doesn't currently need this anymore, and doesn't want to push it forwards, and we feel like it'll need some further discussion and revision.

Given that:

@rfcbot postpone

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 1, 2022

Team member @joshtriplett has proposed to postpone this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Nov 1, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 1, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Nov 1, 2022
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Nov 11, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 11, 2022

The final comment period, with a disposition to postpone, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This is now postponed.

@rfcbot rfcbot added to-announce postponed RFCs that have been postponed and may be revisited at a later time. and removed disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Nov 11, 2022
@rfcbot rfcbot closed this Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
finished-final-comment-period The final comment period is finished for this RFC. postponed RFCs that have been postponed and may be revisited at a later time. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. to-announce
Projects
No open projects
Status: Postponed
Development

Successfully merging this pull request may close these issues.

None yet

10 participants