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

chore(ci): Update Rust versions in CI to 1.74, 1.78, and 1.79 #108

Closed
wants to merge 7 commits into from

Conversation

v0y4g3r
Copy link

@v0y4g3r v0y4g3r commented Oct 6, 2024

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGELOG.md if knowledge of this change could be valuable to users.

This PR changes the rust toolchain for CI according to geo_types because geo_types no longer compiles with current CI settings.

@v0y4g3r
Copy link
Author

v0y4g3r commented Oct 6, 2024

@lnicola @frewsxcv Could you please have a look at this PR?

@v0y4g3r v0y4g3r marked this pull request as draft October 6, 2024 03:13
@v0y4g3r v0y4g3r changed the title chore(ci): Update Rust versions in CI to 1.77, 1.80, and 1.81 chore(ci): Update Rust versions in CI to 1.74, 1.78, and 1.79 Oct 6, 2024
@v0y4g3r v0y4g3r marked this pull request as ready for review October 6, 2024 03:19
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>
on: push
on:
push:
branches: [ "master" ]
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason to only run these on master?

Copy link
Author

Choose a reason for hiding this comment

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

Any particular reason to only run these on master?

No, I just thought it would be nice to let actions only run on importance branches like master. But maybe better to keep the original behavior 14c5816

CHANGELOG.md Outdated
@@ -1,7 +1,7 @@
# CHANGELOG

## Unreleased

- [#108](https://github.com/georust/gpx/pull/108): Update MSRV to 1.74.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't actually bump rust-version in the manifest, though I suspect it still builds on older versions?

Copy link
Author

Choose a reason for hiding this comment

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

This doesn't actually bump rust-version in the manifest, though I suspect it still builds on older versions?

I updated the changelog in f23ed65. This PR only changes the CI toolchain.

Copy link
Member

Choose a reason for hiding this comment

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

"Yes we support older versions, but we don't know how to build them" doesn't make sense to me.

I'd say bump the rust-version, call it an MSRV and move on with your lives.

The alternative you're leading us down here is to have an MSRV that doesn't mean anything to anyone. How would we know when it breaks?

Copy link
Member

@michaelkirk michaelkirk Oct 7, 2024

Choose a reason for hiding this comment

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

To clarify: Whatever is listed as rust-version in Cargo.toml should have a corresponding build in CI. If that means you need to bump rust-version, (I think) that's OK.

Copy link
Member

Choose a reason for hiding this comment

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

But if we switch to rustup, we might not need to bump the MSRV.

@lnicola
Copy link
Member

lnicola commented Oct 7, 2024

Since we're at this, we should also update time to 0.3.35 or 0.3.36.

@lnicola
Copy link
Member

lnicola commented Oct 7, 2024

And do we really need a custom image, or can we build it directly?

@michaelkirk
Copy link
Member

And do we really need a custom image, or can we build it directly?

In the geo and proj repos anyway, we use libproj which takes a while to build, so having a custom image was the way that we cached that build to speed up CI.

Probably you could set something else up though.

Or if you're not using those dependencies, I expect you'd have luck using a standard container.

@v0y4g3r
Copy link
Author

v0y4g3r commented Oct 7, 2024

Since we're at this, we should also update time to 0.3.35 or 0.3.36.

Will do.

And do we really need a custom image, or can we build it directly?

In the geo and proj repos anyway, we use libproj which takes a while to build, so having a custom image was the way that we cached that build to speed up CI.

Probably you could set something else up though.

Or if you're not using those dependencies, I expect you'd have luck using a standard container.

AFAIK the CI does not need a customzied container to do those actions:

  • cargo build --no-default-features
  • cargo test --no-default-features
  • cargo build --all-features
  • cargo test --all-features

@lnicola lnicola mentioned this pull request Oct 8, 2024
2 tasks
@lnicola
Copy link
Member

lnicola commented Oct 8, 2024

Filed #109 as an alternative without the containers.

@lnicola lnicola closed this in #109 Oct 8, 2024
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