Changed how Cargo.toml of baseline is being built, added all features to baseline's manifest

[tonowak]

Fixes #147
Tested on the repo from this issue - seems to work. (Should I write some tests besides manually checking?)

In the issue you mentioned deleting the baseline's Cargo.lock. Is it also needed here? I don't really understand why this step is necessary.

[obi1kenobi]

Excited to merge this, it looks good! Just a few small points worth discussing before we merge.

[obi1kenobi]

I'm not sure, why did you need to add it? :) Would it cause any harm to leave the edition key in the TOML?

Also, consider making these comments as GitHub comments on the PR itself for future PRs? That way we can't miss them either in terms of discussing them or in terms of remembering to remove them later when they've been sorted out.

[tonowak]

I'm not sure, why did you need to add it? :) Would it cause any harm to leave the edition key in the TOML?

I had an error without removing the edition, I'll try to reproduce the error and paste it here when I'll have some time.

Also, consider making these comments as GitHub comments on the PR itself for future PRs? That way we can't miss them either in terms of discussing them or in terms of remembering to remove them later when they've been sorted out.

Sounds reasonable, but then I could forget to comment something after creating the PR...

[obi1kenobi]

Sounds reasonable, but then I could forget to comment something after creating the PR...

You could perhaps keep these notes in a personal local file, and then turn them into comments as you open the PR? That way they aren't in the source code but they are written down before opening the PR.

I usually keep a notes file like this for myself to keep track of everything that's in progress, and I'd recommend it as a process in general.

[tonowak]

I had an error without removing the edition, I'll try to reproduce the error and paste it here when I'll have some time.

Error: Failed when running cargo-doc on ../rust-libp2p/target/semver-checks/registry-libp2p_core-0_37_0/Cargo.toml: error: cannot specify features for packages outside of workspace

[obi1kenobi]

Is it perhaps getting confused about the fact that the target dir is inside the workspace of the other Cargo.toml? Here's what I'd suggest:

• Try reproducing the error with a manual call to cargo from the terminal.
• Then try moving the target directory somewhere else, outside of the workspace of libp2p, and try reproducing the error again with the same call.

I'm a bit worried about the edition removal because it feels like there might be a deeper issue here and the edition is just a symptom rather than the cause.

[tonowak]

The smallest breaking Cargo.toml seems to be

[package]
name = "rustdoc"
edition = "2021"
version = "0.0.0"

[workspace]

[dependencies]
libp2p = "0.37.0"

[lib]
path = "lib.rs"

with an empty lib.rs.

[obi1kenobi]

Re-reading the error message one more time, I think the issue is about attempting to combine --all-features with --package libp2p-core. The libp2p-core package is a dependency of the project and as such its features are specified in the Cargo.toml, so we can't run --all-features on it because what if the Cargo.toml entry for it specifies some other set of features? In other words, --all-features only makes sense in the context of commands applied to the source code corresponding to the Cargo.toml, and does not make sense for dependency packages where features are specified elsewhere.

I think the answer is to not include --all-features in the "make fake project + get rustdoc of the dependency" branch of cargo-semver-checks.

[obi1kenobi]

I think this is the only remaining item for this PR, otherwise it's ready to merge.

[tonowak]

Sure, I'll do it in 2-3h

[obi1kenobi]

No rush, whenever you have a moment is fine.

[obi1kenobi]

Fixes #147 Tested on the repo from this issue - seems to work. (Should I write some tests besides manually checking?)

It would be great to have a test, since this is a tricky issue that would be a pain to debug again if we had a regression.

What do you think would be the best and easiest way to test this? It's a little bit tricky since the issue only happens when using a baseline from crates.io.

In the issue you mentioned deleting the baseline's Cargo.lock. Is it also needed here? I don't really understand why this step is necessary.

Since we'd be emitting a brand new Cargo.toml in the baseline directory, it's probably a good idea to ensure the baseline directory is empty and freshly created, for example by deleting it completely if it already existed.

I'm not sure why I had a problem with Cargo.lock, but the easiest way to not have the problem is to not have any files (including the lockfile) in the baseline directory when writing the new Cargo.toml.

This is another case of "it's better to not have to debug this later." :)

[tonowak]

What do you think would be the best and easiest way to test this? It's a little bit tricky since the issue only happens when using a baseline from crates.io.

It is tempting to add a test that checks for this problem from a selected crate from crates.io, but that would require internet connection. From one side, it seems weird to require access to crates.io during testing, but from the other side, cargo-semver-checks requires it when running the tool. What is your opinion?

[obi1kenobi]

It is tempting to add a test that checks for this problem from a selected crate from crates.io, but that would require internet connection. From one side, it seems weird to require access to crates.io during testing, but from the other side, cargo-semver-checks requires it when running the tool. What is your opinion?

I agree we probably don't want to trigger this at cargo test time, for the reasons you laid out.

How about a test script that gets run on GitHub Actions, as a sort of integration test? For example, it could clone the specific repo and commit where we found this problem, and assert that the false-positive is gone i.e. that running cargo-semver-checks with the appropriate flags exits cleanly.

[tonowak]

So then I would need to edit https://github.com/obi1kenobi/cargo-semver-check/blob/main/.github/workflows/ci.yml right?
I guess the test would clone the rust-libp2p repo with a given relative path and rev and then it would run cargo run on it?

[obi1kenobi]

So then I would need to edit https://github.com/obi1kenobi/cargo-semver-check/blob/main/.github/workflows/ci.yml right? I guess the test would clone the rust-libp2p repo with a given relative path and rev and then it would run cargo run on it?

Yeah, that was what I had imagined, but I'm not tied to that idea or approach. If you have an alternative idea on how to add a regression test for this, I'd love to know about it!

[tonowak]

That idea seems good enough for me. I don't really see another way to test this specific problem without connecting to crates.io during cargo test. And this approach would also potentially find other bugs (connection to crates.io is needed for the tool to work and it is good to check on an example that it works)

[tonowak]

I think I'll add an issue and try to write a new PR for the GitHub Action integration tests and leave this PR without any tests. It seems that those tests would be beneficial not only to this particular PR. What do you think?

[obi1kenobi]

That sounds good. We should just avoid making new releases before those tests are in, which should be okay.