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

release cargo test helper crate to crates-io #10147

Closed
drahnr opened this issue Dec 1, 2021 · 26 comments · Fixed by #13418
Closed

release cargo test helper crate to crates-io #10147

drahnr opened this issue Dec 1, 2021 · 26 comments · Fixed by #13418
Labels
A-cargo-api Area: cargo-the-library API and internal code issues A-testing-cargo-itself Area: cargo's tests C-enhancement Category: enhancement disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review T-cargo Team: Cargo to-announce

Comments

@drahnr
Copy link

drahnr commented Dec 1, 2021

The cargo test helper crate would be a very much appreciated relief for all cargo-$tool plugins, mostly speaking for cargo-unleash and cargo-spellcheck .

(This didn't seem to match any category, hence the free form issue).

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 10, 2021

Do you mean the cargo-test-support or cargo-test-macro crates? They seam pretty customized to our needs. I would love to see how they could be used for your projects. They should be available as git dependencies. Would you be willing to try using them as git dependencies and link us a PR to show how it works for you?

If they turn out to be useful we would still have to agree to what the stability guarantee is. Updating and publishing another set of crates is a nontrivial amount of work. For Cargo's Rust API, we release a new and incompatible version of the library every six weeks. For other crates we put in the effort to track when breaking changes actually occur. So what the release cadence is will need to be figured out.

@alexcrichton
Copy link
Member

For now these crates are not raelly intended for wider consumption. You're welcome, as with all open source, to fork it and customize it for your needs as well. Otherwise though I don't think we want to manage publication of these crates on crates.io.

@Fabian-Gruenbichler
Copy link

would it be possible to re-visit this with a slightly different rationale?

over in the Debian packaging ecosystem, cargo is actually packaged twice:

  • one source package cargo for the binary cargo
  • one source package rust-cargo for the library cargo

the former uses the source tarball published upstream and vendors all the dependencies (with some scripts to remove stuff that is not needed for the purpose of packaging cargo the tool for Debian usage), to avoid the bootstrap cycle of cargo requiring various crates that in turn also require cargo to build. this is basically an excemption that cargo as toolchain package has from the general Debian rule of "no vendoring/code copies". as part of this build, the cargo testsuite is executed.

the latter uses the same tooling as all regular crates packaged for Debian - obtain the source from crates.io, transform Cargo.toml into Debian package and CI metadata, etc.pp.. there are some other crates (most notable, the crate responsible for this transformation, which is called debcargo) which are packaged that use and depend on cargo the library. right now as part of the packaging of rust-cargo, only a regular build is done, since the needed dependencies for running the testsuite are not published on crates.io and thus not packaged for Debian. for the sake of these reverse dependencies, we'd like to run the testsuite not just for src:cargo, but also for src:rust-cargo to catch any regression caused by patches (most commonly, relaxing/upgrading dependencies) on our side.

would it be possible to re-consider publishing cargo-test-support and cargo-test-macro (with the same "eternally-unstable" version number as cargo the lib crate itself)? I know this is a pretty niche use case, but IMHO it's a valid one nevertheless..

@epage
Copy link
Contributor

epage commented Oct 30, 2022

Personally, I think this is worth us reconsidering.

As a cargo team member, I find it frustrating that we do not publish this as it'd be a big help for me for browsing the documentation (I tend to prefer docs.rs over cargo doc --open).

I also have a couple of tools now (cargo-edit, cargo-release) that are using it via a git dependency.

@epage epage reopened this Oct 30, 2022
@Fabian-Gruenbichler
Copy link

are there any blockers for this?

@epage epage added C-enhancement Category: enhancement A-cargo-api Area: cargo-the-library API and internal code issues A-testing-cargo-itself Area: cargo's tests S-needs-team-input Status: Needs input from team on whether/how to proceed. labels Oct 17, 2023
@weihanglo
Copy link
Member

As a cargo team member, I find it frustrating that we do not publish this as it'd be a big help for me for browsing the documentation (I tend to prefer docs.rs over cargo doc --open).

The doc we currently have is on https://doc.rust-lang.org/nightly/nightly-rustc/cargo_test_support/index.html.

For other crates we put in the effort to track when breaking changes actually occur. So what the release cadence is will need to be figured out.

Speaking of publishing this to crates.io. Cargo the project has gained a better support of managing member packages since last year (kudos to cargo-semver-checks and workspace-inheritance feature 😃). I am on the side that we can just publish those two crates with minimal efforts. Yet, the API is not and may never be stable.

@PaulDance
Copy link
Contributor

I also have a couple of tools now (cargo-edit, cargo-release) that are using it via a git dependency.

Same for me, although only one. Having it published would be more comfortable, for sure. It would also help in circumventing #10752 specifically for these packages.

Yet, the API is not and may never be stable.

That should be just fine in my specific case.

@weihanglo
Copy link
Member

weihanglo commented Jan 19, 2024

Some considerations before publishing them:

  • Naming — Are we good with cargo-test-*? Should we follow the new naming rule cargo-util-something?
  • Setting the expectation — We are not going to accept major features for these two crates, unless it also helps clean-up Cargo its own testsuite (in a meaningful sense not cosmetic changes). We should document this somewhere.
  • Stability guarantee — No guarantee on any stability across versions. We would try our best to maintain SemVer compatibility. For behavior changes, we don't want to pay too much attention to it. For example, this might only require a patch version bump but could break people's test code.

@epage
Copy link
Contributor

epage commented Jan 19, 2024

I think the main concern I heard last this came up is the API has a very limited target: you must buy-in to everything.

This is one (of several) reasons I'm pushing on the testing-devex work is so we can have testing vocabulary terms so its easier to drop this cargo test logic into other tests without being limited in testing like is.

Naming — Are we good with cargo-test-*? Should we follow the new naming rule cargo-util-something?

Until we have packages-as-namespaces, the main concern is conflicting with or blocking plugin names. test-support and test-macro seem long and off enough to not be an issue.

@PaulDance
Copy link
Contributor

Problem is, test-macro is already taken. test-support is not, however.

What should therefore be chosen here? Options I see:

  • The current cargo-test-(support|macro): the simplest to implement and easiest to consume as only the source will need to change; a transition to cargo::test-(support|macro) should be possible at a later stage, only requiring a dependency name change.
  • test-(support|macro), at least partially, in order to automatically have cargo::test-(support|macro) once the feature is completed, if I understand this correctly; will require changing the name everywhere used twice I imagine: once when adopting it, including in Cargo itself, and another time when transitioning to packages-as-namespaces.
  • cargo-util-test-(support|macro): same two transitions.

@PaulDance
Copy link
Contributor

Made a tentative implementation assuming the first option. The names should be easy to change, however, in case another option would be preferable in the end.

@weihanglo weihanglo added the T-cargo Team: Cargo label Feb 9, 2024
@weihanglo
Copy link
Member

weihanglo commented Feb 9, 2024

I have no objection to publishing those two crates, but would like to see some warning in their doc like

WARNING: You might not want to use this.

  • This is designed for testing Cargo itself. Use at your own risk.
  • No guarantee on any stability across versions.
  • No feature request would be accepted unless proved useful for testing Cargo.

Let's see how others on the team think about it.

@rfcbot fcp merge

This proposes we publish cargo-test-support and cargo-test-macro to crates.io along with the Rust release process. We won't guarantee stability and won't accept any feature request for these two crates.

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 9, 2024

Team member @weihanglo has proposed to merge 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 An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Feb 9, 2024
@PaulDance
Copy link
Contributor

Thanks for the review!

would like to see some warning in their doc

In their respective README.md, for example? If so, I can add your suggestion easily.

@weihanglo
Copy link
Member

Better in both lib.rs and README.md to me. Let's hold off until FCP ends.

@ehuss
Copy link
Contributor

ehuss commented Feb 10, 2024

One concern I have is the check-version-bump job can be fussy. It puts the onus on the PR author to figure out the version number and understand our release process. It also seems to be a little buggy around the release week, or if you have an old branch (like #12692).

To sidestep that, would it be possible to always do a semver-breaking version bump the same way the cargo crate is bumped? I'm not sure if @weihanglo is still using new-release, but if so we could add it to the version bump there. It would mean there would be new versions published every 6 weeks that may not have any changes, but that seems better than making PR authors do extra work.

@PaulDance
Copy link
Contributor

Maybe documenting this part of the release process further with instructions specific to requiring bumping a version could be enough instead?

A concern I have is that, contrary to cargo-the-library, the contents of the smaller crates don't often change, meaning empty releases would be created, like you mentioned. Would it happen often, though?

@epage
Copy link
Contributor

epage commented Feb 11, 2024

A concern I have is that, contrary to cargo-the-library, the contents of the smaller crates don't often change, meaning empty releases would be created, like you mentioned. Would it happen often, though?

Every release because the MSRV gets bumped every release. When MSRV RFC is approved and rust-version = auto becomes available, it'll go to an on-need basis. A lot of this is an artifact of the automation we've put in to prevent mistakes.

Speaking of, if we publish this, the git2 dependency becomes quite important and we likely want to add to check-versions that any breaking change in a workspace dependency causes the package to need to be bumped so that we can align on the sys crate versions with users as they upgrade.

@weihanglo
Copy link
Member

@ehuss
To not confuse contributors, we could move the version check job to a cronjob, or triggered by every non PR commit. And then it can open a PR and post to a Zulip topic if a bump is needed, just like what miri-cron-bot does. Does it sound better?

@PaulDance
Copy link
Contributor

Every release because the MSRV gets bumped every release. When MSRV RFC is approved and rust-version = auto becomes available, it'll go to an on-need basis. A lot of this is an artifact of the automation we've put in to prevent mistakes.

Just to be clear -- sorry if I wasn't, I meant to ask about the frequency of such empty releases, i.e. ones that don't include any kind of change in it; a non-empty release would be one with at least one commit during the six-week period. Thanks for the additional info.

Manually looking at the logs from GitHub Web -- I don't have a better option at the moment, a very rough estimate is that in the past year, the cargo-platform and cargo-test-macro crates had two instances of commits being separate by more than six weeks and home, cargo-test-support, crates-io did not. Putting them all in a "rolling release" like cargo should therefore be fine for now as empty releases wouldn't happen too often.

@ehuss
Copy link
Contributor

ehuss commented Feb 12, 2024

To not confuse contributors, we could move the version check job to a cronjob, or triggered by every non PR commit. And then it can open a PR and post to a Zulip topic if a bump is needed, just like what miri-cron-bot does. Does it sound better?

I think it depends if we end up having stable crates that we maintain for public consumption (like the build.rs helper). In that case, I think it is useful to have these checks in PR because we do want to be careful about accidental semver-breaking changes, and those can sometimes be hard to see during review.

But I think I would be fine with separating the job. Worst case we can revert semver-breaking changes if we decide those were unintentional.

I think another concern is that it just shifts the burden onto someone else (you?) to deal with the fussy task of bumping versions. If you're ok with that, then it seems fine with me.

@epage
Copy link
Contributor

epage commented Feb 12, 2024

Can we have the Action bump the versions in the PR? We can then review the bumps and suggest a change and commit it ourself before merging. I mention this because its the workflow I want for changelogs.

@weihanglo
Copy link
Member

I mention this because its the workflow I want for changelogs.

Could you elaborate on this?

@epage
Copy link
Contributor

epage commented Feb 19, 2024

This is getting back to epage/epage.github.io#23

@weihanglo
Copy link
Member

Can we have the Action bump the versions in the PR? We can then review the bumps and suggest a change and commit it ourself before merging. I mention this because its the workflow I want for changelogs.

I also found this quite useful for checking what has changed when doing #13494, so I slightly lean toward the current workflow.

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Mar 19, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 19, 2024

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

@weihanglo weihanglo added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-needs-team-input Status: Needs input from team on whether/how to proceed. labels Mar 19, 2024
@bors bors closed this as completed in 3a77350 Mar 26, 2024
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Mar 29, 2024
charmitro pushed a commit to charmitro/cargo that referenced this issue Sep 13, 2024
As discussed in rust-lang#10147.

Signed-off-by: Paul Mabileau <paulmabileau@hotmail.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cargo-api Area: cargo-the-library API and internal code issues A-testing-cargo-itself Area: cargo's tests C-enhancement Category: enhancement disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review T-cargo Team: Cargo to-announce
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants