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 package --no-verify fails if a package's version req is too high for the registry (but works locally) #15059

Open
weihanglo opened this issue Jan 13, 2025 · 35 comments · May be fixed by #15234
Labels
C-bug Category: bug Command-package regression-from-stable-to-stable Regression in stable that worked in a previous stable release. S-triage Status: This issue is waiting on initial triage.

Comments

@weihanglo
Copy link
Member

weihanglo commented Jan 13, 2025

Problem

#13447 dd698ff has some unintentional consequences:

cargo package after that generates a lockfile also when packaging libraries.
However, if the crate being packaged depends on some unpublished dependencies. When generating the lockfile Cargo then can't find unpublished deps on registry index, so it bailed out.

Steps

This could be simplly reproduced in rust-lang/cargo as of today (2025-01-13)

  1. Checkout to 54df3c7
  2. Bump cargo-test-support to an unpublished version such as 99.99.99
  3. cargo +1.83.0 package -p cargo-test-support --no-verify — succeeded
  4. cargo +1.84.0 package -p cargo-test-support --no-verify — failed

Possible Solution(s)

Assume if we have passed the local registry unconditionally, we shouldn't have the issue today.

let mut local_reg = if ws.gctx().cli_unstable().package_workspace {
let reg_dir = ws.target_dir().join("package").join("tmp-registry");
sid.map(|sid| TmpRegistry::new(ws.gctx(), reg_dir, sid))
.transpose()?
} else {
None
};

Version

  • cargo 1.84.0 (66221abde 2024-11-19)
  • cargo 1.86.0-nightly (088d49608 2025-01-10)
@weihanglo weihanglo added C-bug Category: bug Command-package regression-from-stable-to-stable Regression in stable that worked in a previous stable release. S-triage Status: This issue is waiting on initial triage. labels Jan 13, 2025
@epage
Copy link
Contributor

epage commented Jan 13, 2025

So this is a problem that existed before 54df3c7 but only for some crates. Now all see it, right?

@weihanglo
Copy link
Member Author

Yeah I believe it was only bugging for packages having [[bin]] crates, but now every package.

@epage
Copy link
Contributor

epage commented Jan 13, 2025

@Eh2406 our resolve call is

let mut new_resolve = ops::resolve_with_previous(
&mut tmp_reg,
&tmp_ws,
&CliFeatures::new_all(true),
HasDevUnits::Yes,
orig_resolve.as_ref(),
None,
&[],
true,
)?;

Is there a way to say "prefer existing but don't lock them? Would keep = |_| false do that? Seems like what we'd want (have to double check yanked) and then cargo package already has logic to warn in these situations which imo is good because its easy to accidentally release a package and not realize you forgot to release a dependency.

@weihanglo
Copy link
Member Author

In terms of a swift fix: How stable is TmpRegistry?
It might be helpful if we could use it internally without stabilizing the entire -Zpackage-workspace.

@epage
Copy link
Contributor

epage commented Jan 13, 2025

I'm not sure how that would fix it. We wouldn't be putting the package into TmpRegistry to be able to be used unless the user requests it at which point it will work anyways.

@weihanglo
Copy link
Member Author

More like Cargo automatically publishes unpublished dependencies crates when needed to TmpRegistry before building the lock?

@epage
Copy link
Contributor

epage commented Jan 14, 2025

I suspect getting it right to sometimes publish to TmpRegistry can cause us other problems and would get in the way of stabilizing workspace packaging that I feel like that less likely to be our best option for a swift fix.

if we feel the need to backport to beta, a revert is likely the best route. If not, we should at least understand what it would take on the resolver side.

@sehz
Copy link

sehz commented Jan 14, 2025

We are running into same issue. Can this is be reverted?

@epage epage changed the title cargo package failed if a package contains unpublished local dependencies cargo package --no-verify fails if a package's version req is too high for the registry (but works locally) Jan 14, 2025
@epage
Copy link
Contributor

epage commented Jan 14, 2025

I cannot reproduce this if I bump the package's version but don't bump the version requirement. I also missed the detail that --no-verify is required..

This means that cargo is able to adjust the lockfile correctly as needed and the problem is that we moved "dependency exists" checks from the verify step to the packaging step, forcing it to always happen.

The documentation for cargo publish --no-verify just says

Don’t verify the contents by building them.

I think this still leaves room for other verification. Historically, we've done some registry and dependency verification and we recently added "is this published?" (#14448).

let just_pkgs: Vec<_> = pkgs.iter().map(|p| p.0).collect();
let reg_or_index = match opts.reg_or_index.clone() {
Some(r) => {
validate_registry(&just_pkgs, Some(&r))?;
Some(r)
}
None => {
let reg = super::infer_registry(&just_pkgs)?;
validate_registry(&just_pkgs, reg.as_ref())?;
if let Some(RegistryOrIndex::Registry(ref registry)) = &reg {
if registry != CRATES_IO_REGISTRY {
// Don't warn for crates.io.
opts.gctx.shell().note(&format!(
"found `{}` as only allowed registry. Publishing to it automatically.",
registry
))?;
}
}
reg
}
};
// This is only used to confirm that we can create a token before we build the package.
// This causes the credential provider to be called an extra time, but keeps the same order of errors.
let source_ids = super::get_source_id(opts.gctx, reg_or_index.as_ref())?;
let (mut registry, mut source) = super::registry(
opts.gctx,
&source_ids,
opts.token.as_ref().map(Secret::as_deref),
reg_or_index.as_ref(),
true,
Some(Operation::Read).filter(|_| !opts.dry_run),
)?;
{
let _lock = opts
.gctx
.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?;
for (pkg, _) in &pkgs {
verify_unpublished(pkg, &mut source, &source_ids, opts.dry_run, opts.gctx)?;
verify_dependencies(pkg, &registry, source_ids.original)?;
}
}

Its also not clear to me why you would be publishing packages without dependencies present. Is this a workaround for #4242? We already have a method for this for some cases (leaving off versions for dev-dependencies). If there is a case that doesn't cover, it would be good to make sure its reported in #4242.

@weihanglo
Copy link
Member Author

weihanglo commented Jan 14, 2025

Note that this is about cargo package not publish.

There are workflows packaging source code into .crate files before any publishment. Not going to judge in this comment whether those workflows should be supported by Cargo, but the behavior did change in 1.84.

This Cargo-flavored test works in 1.83 but fails in 1.84.

#[cargo_test]
fn unpublished_dependency() {
    Package::new("dep", "0.1.0").publish();

    let p = project()
        .file(
            "Cargo.toml",
            r#"
            [package]
            name = "main"
            version = "0.0.1"
            edition = "2015"

            [dependencies]
            dep = { path = "dep", version = "0.1.1" }
        "#,
        )
        .file("src/lib.rs", "")
        .file(
            "dep/Cargo.toml",
            r#"
            [package]
            name = "dep"
            version = "0.1.1"
            edition = "2015"
        "#,
        )
        .file("dep/src/lib.rs", "")
        .build();

    p.cargo("package --package main --no-verify --no-metadata")
        .with_status(101)
        .with_stderr_data(str![[r#"
[PACKAGING] main v0.0.1 ([ROOT]/foo)
[UPDATING] `dummy-registry` index
[ERROR] failed to prepare local package for uploading

Caused by:
  failed to select a version for the requirement `dep = "^0.1.1"`
  candidate versions found which didn't match: 0.1.0
  location searched: `dummy-registry` index (which is replacing registry `crates-io`)
  required by package `main v0.0.1 ([ROOT]/foo)`
  perhaps a crate was updated and forgotten to be re-vendored?

"#]])
        .run();
}

@epage
Copy link
Contributor

epage commented Jan 14, 2025

That seems like a very limited workflow as it only worked if the crates had no [[bin]]s and no [[example]]s in them. Once you add one, it failed.

@epage
Copy link
Contributor

epage commented Jan 14, 2025

@sehz could you help us understand more about your use case? I feel like there are reasons to keep the new behavior but I want to also better understand your situation to better understand how compelling the old behavior is.

@sehz
Copy link

sehz commented Jan 14, 2025

We are building development version of binary which depends on unpublished version of crates for obvious reason. The binary is not in the same repo as other crates. So we packaged unpublished version of crates so it can be refer by binary crate.

@sehz
Copy link

sehz commented Jan 14, 2025

Would be ok to have previous behavior with flag.

@epage
Copy link
Contributor

epage commented Jan 15, 2025

Could you go into more detail. I feel I'm not quite getting why cargo package is being used and particularly why you need to package when dependencies are unavailable.

@nappa85
Copy link

nappa85 commented Jan 31, 2025

I'm having the same problem, my use case is that the package is used only internally (private registry) and it depends on git thirdparty repos that are not published. Mirroring those repos on private registry would be a burden, they are more than one hundred between direct dependencies and sub-dependencies, some requires a specific git commit of a crate that is published, so I prefer to insert the deps with a patch on binary's Cargo.toml as a temporary solution, but Rust 1.84.0 broke my pipeline

jpraynaud added a commit to input-output-hk/mithril that referenced this issue Feb 6, 2025
In CI and Pre-release workflows.

This is due to a change of bahavior in cargo since version
See rust-lang/cargo#15059

Will be rolled back after release of distribution '2506'
@cpg314
Copy link

cpg314 commented Feb 13, 2025

Another illustration is https://github.com/cpg314/cargo-depot: see this section of the README.
I was using cargo package, disabling examples and binaries to prevent lockfile regeneration, with a similar use case as #15159.
This is not possible anymore after dd698ff

@sehz
Copy link

sehz commented Feb 13, 2025

Our use case is that we use cargo package to distribute unpublished crates for beta version. crates.io is not designed for distributing dev version. This is preventing us from upgrading to latest Rust version.

@epage
Copy link
Contributor

epage commented Feb 18, 2025

Could the people running into this check and report back on whether -Zpackage-workspace resolves the problem they are running into. That allows multiple packages within a workspace to packaged in one call in the correct order. If not, could you explain why not?

@weihanglo
Copy link
Member Author

It is a bit tricky to verify my use case since both libcargo and cargo CLI are involved.

I believe -Zpackage-workspace would help with most of my use cases. However, it would not help with the case that people already used --package to select certain packages in certain order. For example, if crate a depends on crate b.

  • cargo -Zpackage-workspace package -p a && cargo -Zpackage-workspace package -p b would fail because b wasn't in the temporary registry index created by -Zpackage-workspace when it published a.
  • cargo -Zpackage-workspace package -p b && cargo -Zpackage-workspace package -p a would suceed because two invocation shared the same temp registry index, and when it published a it saw b in index.

So with the current implementation of -Zpackage-workspac, the issue is only partially fixed.

@epage
Copy link
Contributor

epage commented Feb 18, 2025

Could those uses be switched to cargo -Zpackage-workspace package -p a -p b? That was at least the intent behind my suggestion of -Zpackage-workspace.

@morenol
Copy link

morenol commented Feb 18, 2025

I still get same error, which is related to packaging a crate that has as dependency other internal dependencies that are not yet released

Packaging <plg-name <version> (path)
   Updating crates.io index
error: failed to prepare local package for uploading

Caused by:
  failed to select a version for the requirement `internal-deps = \"^0.12.0\"`
 candidate versions found which didn't match: 0.11.0
  location searched: crates.io index
  required by package `<pkg-name>

@epage
Copy link
Contributor

epage commented Feb 18, 2025

@morenol could you expand on your use case?

@morenol
Copy link

morenol commented Feb 18, 2025

We have a binary that generates a rust project. Normally, the project generated uses crates.io packages, but while we are developing unstable features we pack the crates and embed them in the binary so the code generator can generate code the references these non released packages

@weihanglo
Copy link
Member Author

Could those uses be switched to cargo -Zpackage-workspace package -p a -p b? That was at least the intent behind my suggestion of -Zpackage-workspace.

That could be one solution, yes. However, it then becomes a user problem of my tool, which they need to intervene their broken builds (not too hard but still a churn).

@weihanglo
Copy link
Member Author

I still get same error

@morenol, did you mean it still failed with -Zpackage-workspace on? Could you show the redacted version of the Cargo command your tool invoked?

@morenol
Copy link

morenol commented Feb 18, 2025

This is call in a build.rs,

    let status = Command::new("cargo")
        .arg("-Zpackage-workspace")
        .arg("package")
        .arg("-p")
        .arg(name)
        .arg("--target")
        .arg(WASI_TARGET)
        .arg("--all-features")
        .arg("--no-verify")
        .arg("--allow-dirty")
        .arg("--target-dir")
        .arg(out_dir)
        .output()

@weihanglo
Copy link
Member Author

@morenol
While within build script you can do whatever you like, I truly sympathize you need to do such workflow in build.rs. Can you try packaging everything in one cargo package call, or using --workspace instead of -p for packaging the entire workspace?

@weihanglo
Copy link
Member Author

weihanglo commented Feb 25, 2025

To summarize, cargo package --no-verify at least fails in three different cases after #13447:

  • An unpublished package depending on itself as a dev-dependency (cyclic self-referential dev-dependencies).
    • Can be resolved by removing the version field from the affected dev-dependency.
    • -Zpackage-workspace has the same behavior as without the flag, so doesn't help fix the failure.
  • Existing cargo package has --package <pkg> specifying some but not all unpublished packages.
    • Can be resolved by specifying all unpublished packages in one cargo call.
    • -Zpackage-workspace requires all dependency versions available in the target registry or specified through the -p flag when packaging. You still need to specify all unpublished when using -Zpackage-workspace.
  • cargo package --no-verify has been used as a kind of “plumbing commands” to create tarballs without considering dependency orders. The use cases include:
    • Preparing tarballs for other package managers.
    • Integrating into custom develop workflows for unpublished/internal crates.
    • Constructing custom/private registries.

@weihanglo
Copy link
Member Author

One question continues popping up: why it wasn't a problem for [[bin]] until cargo started generating lockfile for [lib].

Here is my answer:

A lot of packages on crates.io and in the wild are lib only crates. People learn to use Cargo workspaces to separate bins from lib packages for a better dependency management (you can't have bin-only dependency in one package). So [[bin]] is less a problem, but it still a problem. People could work around it, like removing [[bin]] from Cargo.toml if there is no interest. And most packagers doing cargo package --no-verify cares more about libraries. If it were for binaries usually they just build them won't “package” the source.


I think upon this moment, as -Zpackage-workspace might not fit every need here, especially for packaging circular dev-dependencies, I would personally favor the proposal in #15159.

@epage
Copy link
Contributor

epage commented Feb 25, 2025

Existing cargo package has --package specifying certain unpublished packages.

  • -Zpackage-workspace also requires all dependency versions available in the target registry when calling, so doesn't help.

I think some context to this summary item is missing because I'm not following this.

And most packagers doing cargo package --no-verify cares more about libraries. If it were for binaries usually they just build them won't “package” the source.

If we're talking doing weird things for first-party code, then all of this is being done for the sake of releasing a [[bin]].

If the process involves common crates.io dependencies, then at least clap will run into this because it as a bin for testing.

@weihanglo
Copy link
Member Author

  • -Zpackage-workspace also requires all dependency versions available in the target registry when calling, so doesn't help.

I think some context to this summary item is missing because I'm not following this.

Sorry for that. The comment just got updated. Let me know if it is still not clear.

If we're talking doing weird things for first-party code, then all of this is being done for the sake of releasing a [[bin]].

If the process involves common crates.io dependencies, then at least clap will run into this because it as a bin for testing.

The process can be more complicated when using custom registries or other approaches for sharing code through tarballs. [[bin]] does not need to be there in the first place when packaging libraries. It could be a separate bin package for final production. And packages on crates.io may have a different way to consume than first-party code/crates. People chimed in this thread has already provided the oddity of their environments.

@morenol
Copy link

morenol commented Feb 25, 2025

  • Can be resolved by specifying all unpublished packages in one cargo call.

How would that be done? Do you have an example?

@weihanglo
Copy link
Member Author

How would that be done? Do you have an example?

It depends on whether you know what you're going to publish beforehand. For example, if you have a names slice of all packages you're going to package, and they are in the same workspace, you shall be able to do something like

let cmd = Command::new("cargo");
cmd.arg("package")
    .arg("-Zpackage-workspace")
    .arg("--target")
    .arg(WASI_TARGET);
for name in names {
    cmd.arg("-p").arg(name);
}
let output = cmd.output()

@morenol
Copy link

morenol commented Feb 25, 2025

ok, that seems to work now for me using the +nightly channel.

@weihanglo weihanglo linked a pull request Feb 25, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug Command-package regression-from-stable-to-stable Regression in stable that worked in a previous stable release. S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants