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

False positive: baseline rustdoc built from registry version only includes default features #147

Closed
obi1kenobi opened this issue Oct 15, 2022 · 3 comments · Fixed by #173
Assignees
Labels
C-bug Category: doesn't meet expectations

Comments

@obi1kenobi
Copy link
Owner

obi1kenobi commented Oct 15, 2022

When cargo-semver-checks constructs a baseline rustdoc JSON file, it (implicitly) activates the default features only. The "current" rustdoc is instead constructed with --all-features.

If a feature e.g. adds an enum variant, that variant is detected as being newly-added: exists in current, does not in baseline. This causes false-positive semver errors.

Example affected crate: libp2p-core v0.37.0

$ cargo semver-checks check-release -p libp2p-core
    Updating index
     Parsing libp2p-core v0.37.0 (current)
     Parsing libp2p-core 0.37.0 (baseline)
    Checking libp2p-core v0.37.0 -> v0.37.0 (no change)
   Completed [   1.094s] 18 checks; 17 passed, 1 failed, 0 skipped

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.12.0/src/queries/enum_variant_added.ron

Failed in:
  variant PublicKey:Rsa in core/src/identity.rs:223
  variant PublicKey:Secp256k1 in core/src/identity.rs:226
  variant PublicKey:Ecdsa in core/src/identity.rs:229
  variant PublicKey:Rsa in core/src/identity.rs:223
  variant PublicKey:Secp256k1 in core/src/identity.rs:226
  variant PublicKey:Ecdsa in core/src/identity.rs:229
  variant Keypair:Rsa in core/src/identity.rs:73
  variant Keypair:Secp256k1 in core/src/identity.rs:76
  variant Keypair:Ecdsa in core/src/identity.rs:79
       Final [   1.175s] semver requires new major version: 1 major and 0 minor checks failed

The equivalent call using --baseline-rev reports no errors: cargo semver-checks check-release -p libp2p-core --baseline-rev libp2p-core-v0.37.0

Additional info:

@obi1kenobi obi1kenobi added the C-bug Category: doesn't meet expectations label Oct 15, 2022
@obi1kenobi obi1kenobi changed the title Possible bug: no code changes but semver issues found in feature-gated code. False positive: baseline rustdoc built from registry version only includes default features Oct 16, 2022
@obi1kenobi
Copy link
Owner Author

@epage is loading the list of features from the index and adding it to the dependency line a good idea?

Or is it better to try to match the crate version to a tag in the git repo using a set of heuristics like v{version} or {crate_name}-{version}?

I don't fully understand the code around there — especially how we manage to get a crate's rustdoc by making a new manifest by declaring a dependency on it — so I'd love your take on how to go about this.

@epage
Copy link
Collaborator

epage commented Oct 16, 2022

@epage is loading the list of features from the index and adding it to the dependency line a good idea?

Ideally we'd centralize our handling of features (RustDocCommand does --all-features iirc) but absent that, then yes, that would be the best way to handle that.

especially how we manage to get a crate's rustdoc by making a new manifest by declaring a dependency on it — so I'd love your take on how to go about this.

When we run rustdoc, we pass in <name>@<version> to request one specific crate's documentation from the dependency tree, the dependency.

@obi1kenobi
Copy link
Owner Author

Ideally we'd centralize our handling of features (RustDocCommand does --all-features iirc) but absent that, then yes, that would be the best way to handle that.

I think that --all-features ends up only applying to the root crate (named rustdoc) and not to the dependencies.

I was able to confirm by hand that the following sequence works:

That's my plan for moving forward since it seems to work well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: doesn't meet expectations
Projects
None yet
2 participants