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

run tests in CI #675

Merged
merged 2 commits into from
Jun 14, 2023
Merged

run tests in CI #675

merged 2 commits into from
Jun 14, 2023

Conversation

danieleades
Copy link
Contributor

@danieleades danieleades commented Jan 31, 2023

  • refactors the CI workflow to group targets by action (check or test) rather than by toolchain
  • converts the 'check' targets into a unified matrix target
  • adds a 'test' target to run the tests in CI
  • runs the checks in parallel instead of in sequence
  • adds an '--all-targets' check on stable to check the bench and test targets

outstanding questions-

  • are all of these matrix targets needed, or can this be trimmed down further?

I believe merging this will unblock adding configuration to allow dependabot to bump rust deps in this repo, since the CI will catch breaking changes in dev dependencies with these changes. see #674

@danieleades danieleades marked this pull request as draft January 31, 2023 11:45
@danieleades danieleades marked this pull request as ready for review January 31, 2023 11:51
Copy link
Member

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

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

bors r+

@jswrenn jswrenn added this to the next milestone Jun 14, 2023
@bors
Copy link
Contributor

bors bot commented Jun 14, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit c81f80b into rust-itertools:master Jun 14, 2023
@Philippe-Cholet
Copy link
Member

Coming from #720, I'm surprised to see we test againt rust 1.62.1 and not rust 1.36.0. I do not see a discussion about this change.
Does that mean the MSRV is updated? Then update the module doc of "lib.rs" and rust-version in Cargo.toml?
I guess we were gonna update that soon anyway.

@jswrenn
Copy link
Member

jswrenn commented Sep 17, 2023

I noticed that too while reviewing. It's unfortunate that happened. My guess is that a dev-dependency has a higher MSRV. I'll reorganize our CI again soon and address this.

@danieleades
Copy link
Contributor Author

I noticed that too while reviewing. It's unfortunate that happened. My guess is that a dev-dependency has a higher MSRV. I'll reorganize our CI again soon and address this.

having a look at this now. Your guess is correct- it's Criterion that's pushing up the MSRV.
You'd need a way to exclude the dev dependencies when running cargo check, as they are built and checked by cargo by default (even if running cargo check --lib)

Looks like cargo hack might provide a way forward?

@Philippe-Cholet
Copy link
Member

Alternatively, maybe we could just decrease criterion version as needed for a little while? I don't see any recent changes in "benches" that would rely on a newer version.
In the recent #702, it was briefly discussed to increase the MSRV (implicitely soon enough). We could then just increase versions back?

@danieleades
Copy link
Contributor Author

Alternatively, maybe we could just decrease criterion version as needed for a little while? I don't see any recent changes in "benches" that would rely on a newer version.

that's a temporary fix, but you'll still have the general problem that downstream users are unnecessarily restricted to the MSRV of the dev dependencies used by the this crate. A solution that decouples downstream users from developers would be preferable

@danieleades
Copy link
Contributor Author

see #754

@jswrenn jswrenn modified the milestones: next, v11.0.0 Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants