Skip to content
This repository has been archived by the owner on Nov 5, 2022. It is now read-only.

dev-dependencies are annoying and almost useless in publish #46

Closed
Eh2406 opened this issue Jun 26, 2019 · 3 comments
Closed

dev-dependencies are annoying and almost useless in publish #46

Eh2406 opened this issue Jun 26, 2019 · 3 comments

Comments

@Eh2406
Copy link

Eh2406 commented Jun 26, 2019

The inspiring case is that Cargo itself just added custom proc-macro ( rust-lang/cargo#6900 ) to remove testing boilerplate. Now we can't publish to Crates.io, as path dependencies are not allowed, without publishing the small testing library. But noone using Cargo as a library needs to know that we use this dep for testing. We have checked carefully and Cargo only looks at dev-dependencies for the main project and completely ignores them for all dependencies. @alexcrichton, suggested that we comment that part of our Cargo.toml out before we publish, it works for his other projects. We are discussing ways to generalize that workflow.

Possible next steps:

  • leave things as they are. Each project finds its own workaround as needed. Or.
  • Cargo just removes dev-dependencies part of the Cargo.toml before giving it to Crates.io as part of publish for everyone. (This is not great as Crater uses that Cargo.toml, so it would be nice if testing worked from that file.) Or.
  • Cargo adds a flag to strip dev-dependencies part of the Cargo.toml as part of publish only when asked to. (not great as it is an additional CLI flag to doc and maintain) Or.
  • Crates.io and Cargo decide that you can publish with arbitrarily bad data in the dev-dependencies part of the Cargo.toml. Or.
  • Some compromise between them.

We wanted to get the Crates.io Teams thoughts about Cargo not telling you things that we historically did or about changing your policy.
Specifically, do you use dev-dependencies for anything? If so would you mind not using them?
Are you open to allowing path and git dependencies in the dev-dependencies for crates on the site?

This was brought up at #43
Then followed up Cargo team meeting notes:

2019-06-26

  • follow up from crates.io team on not checking dev deps

Both teams want to make this easier but want to think carefully about how to proceed. Let's have a discussion.

@carols10cents
Copy link
Member

can we allow path = "subdir" but disallow path="../other-dir”?

This was my off-the-cuff suggestion: if dev-deps are path deps that are in the repo and included in the .crate, they could be allowed without breaking crater. Is this a possibility or is there something I could be missing about the feasibility/usefulness of this?

@carols10cents
Copy link
Member

Discussion in the crates.io meeting today:

sgrif Today at 4:02 PM
I think the idea of including them in the .crate file is a nice extension, but right now if we don't allow it folks are just going to comment it out to publish, so things are just as broken for crater regardless of whether we reject them or not
@Eh2406 Was there anything that came out of the Cargo meeting on this that's missing? The Cargo team minutes more or less seem like just a summary of the last time this was discussed in the crates.io team meeting

Eh2406 Today at 4:03 PM
Not really.

carols10cents Today at 4:03 PM
is the cargo team looking for action from us?

Eh2406 Today at 4:04 PM
determining if the path dep will be in the .crate exactly will be hard, but we can approximate it easily.

sgrif Today at 4:04 PM
I think we should just allow it either way, since in cases where folks are already using a relative path they'll still just comment it out.
That said, I don't feel strongly and have no issue with requiring the path be relative to the crate root if other folks feel strongly that should be required

Eh2406 Today at 4:05 PM
In terms of action, if you want to just allow it then cargo don't need to change.

carols10cents Today at 4:06 PM
would we display working or not working dev deps on the crate's page? or just not change that either and ignore them completely

Eh2406 Today at 4:06 PM
If you are not going to allow it then you don't need an action. but cargo will start designing a way to make this easier.

carols10cents Today at 4:07 PM
do people care if their crate gets built with crater or not?
are people able to tell if their crate can be built with crater or not?

Eh2406 Today at 4:07 PM
My thought is that Crates.io would just ingor dev-dep that are not from crates.i0

sgrif Today at 4:08 PM
I'm not sure about the sidebar -- I think we should still display at least the name, and probably still the version (range) if there's one given. If there isn't one I believe Cargo reports it as *, not sure if we want to display that or not. This is also a detail we can probably work out later

pietroalbini Today at 4:08 PM
> are people able to tell if their crate can be built with crater or not?
@carols10cents not without looking hard today, but changing this is on my never-ending todo list

sgrif Today at 4:08 PM
We could also just ignore it, yeah

Eh2406 Today at 4:08 PM
Some do care. And I'd like the defalt to be that it works.

sgrif Today at 4:08 PM
People can also just publish a crate that requires certain features to run tests, or are completely broken in #[cfg(test)] which would also break crater in the same way

Eh2406 Today at 4:09 PM
But lots care about other things more.

sgrif Today at 4:09 PM
I agree that we should make things work with crater if we can
I'm just not sure it's a great metric for us to filter publishes on, since folks will just publish something that can't have tests run anyway
@Eh2406 So it sounds like ultimately what the cargo team needs from us is a final decision on whether we are willing to just start allowing paths for dev-dependencies or not?

Eh2406 Today at 4:11 PM
Yes
Or what you will except if it is not everything

sgrif Today at 4:12 PM
Great. Unless anyone has something they'd like to bring up synchronously, let's continue the discussion on the specifics on https://github.com/rust-lang/crates-io-cargo-teams/issues/46 and try to reach consensus next week

@sgrif
Copy link
Contributor

sgrif commented Jul 25, 2019

This was discussed in the crates.io team meeting today. An issue has been opened to support this in crates.io at rust-lang/crates.io#1789.

The consensus was:

  • We will allow path dependencies for dev-dependencies only.
    • This implies we will allow * ranges, but only if a path field is present
  • We will continue to disallow git dependencies
  • The path will be validated on crates.io to the best of our ability to ensure that it's within the crate itself (most likely by requiring it to be a relative path, and disallowing any segment to be ..)
    • Additional validations, such as ensuring the file is not in .gitignore or excluded should live in Cargo.
  • We will not require the crate to actually exist in our database, and will make the appropriate changes to ensure this is the case
    • This actually will probably just mean we don't store them at all (sgrif has since verified that we do not use them for anything other than UI and reverse-dependencies, so we don't need to store these at all)
  • Path dependencies are not included for reverse-dependency purposes, nor are they displayed in the UI

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants