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

Ensure the lint files are part of the binary file itself. #234

Merged
merged 1 commit into from Dec 19, 2022

Conversation

obi1kenobi
Copy link
Owner

As part of the add_lints!() macro, we had accidentally made the lints no longer be part of the cargo-semver-checks binary itself. This means that everything worked fine as long as it was run from the cargo-semver-checks repo itself, and failed spectacularly as soon as that was not the case (e.g. after cargo install cargo-semver-checks).

This PR re-adds the lints into the binary using include_str!().

@obi1kenobi obi1kenobi added A-cli Area: engine around the lints C-bug Category: doesn't meet expectations labels Dec 19, 2022
@obi1kenobi obi1kenobi enabled auto-merge (squash) December 19, 2022 21:27
@tonowak
Copy link
Collaborator

tonowak commented Dec 19, 2022

What :O

@obi1kenobi obi1kenobi merged commit 0144c3a into main Dec 19, 2022
@obi1kenobi obi1kenobi deleted the include_the_lints branch December 19, 2022 21:34
@tonowak
Copy link
Collaborator

tonowak commented Dec 19, 2022

I see. Have you already thought about possible tests that could catch this?

@obi1kenobi
Copy link
Owner Author

obi1kenobi commented Dec 19, 2022

What :O

I know, right? At compile-time, include_str!() has a stable . directory, so in my code review I completely missed that we had switched to a runtime-evaluated std::fs::read_to_string() with a relative path, which could point anywhere since the program itself doesn't attempt to set it to anything. And since we also don't bundle the lint files as part of the build, this was never going to work.

I found it this by accident while testing something else with the locally-built binary for the newest version, so we got lucky. Otherwise, I think this would have made it through our entire release process and would have broken our users.

It's probably a good idea to add a CI check that does cargo build, then does a cd into the release directory and runs the built binary directly on some rustdoc JSON files (e.g. from the test suite). If we put it after the cargo test step then it probably will have minimal cost, since cargo test will already have done almost the entire build.

@obi1kenobi
Copy link
Owner Author

Would you be interested in adding that CI check, or should I go for it?

Comment on lines +321 to +322
// No way to avoid this lint -- the push() calls are macro-generated.
#[allow(clippy::vec_init_then_push)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a new clippy lint? Should I update my clippy?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's new, it's been there since 1.51 per this page: https://rust-lang.github.io/rust-clippy/master/index.html#vec_init_then_push

The old code didn't trigger that lint for me, though. Not sure why only the new code gets it.

src/query.rs Show resolved Hide resolved
src/query.rs Show resolved Hide resolved
@tonowak
Copy link
Collaborator

tonowak commented Dec 19, 2022

Would you be interested in adding that CI check, or should I go for it?

I can try doing that this week. Will you create an appropriate issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: engine around the lints C-bug Category: doesn't meet expectations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants