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

Fix Rust 1.80 cfg Linting and add VolumeRef impls #161

Merged
merged 5 commits into from
Aug 2, 2024

Conversation

gierens
Copy link
Contributor

@gierens gierens commented Jul 28, 2024

Rust 1.80 which is used in functional tests does more extensive cfg linting.
Because volume and identity are not known it causes functional tests to fail.
This adds a linting rule to allow both those values.

For more info see: https://blog.rust-lang.org/2024/07/25/Rust-1.80.0.html#checked-cfg-names-and-values

@gierens
Copy link
Contributor Author

gierens commented Jul 28, 2024

1.65 tests are failing due to tokio requiring rust 1.70, see #159 for fix

@gierens
Copy link
Contributor Author

gierens commented Jul 28, 2024

Zed fails, but is EOL anyway, see #160

@dtantsur
Copy link
Owner

dtantsur commented Aug 1, 2024

@gierens hi! Shouldn't we add them to the feature list if we use them?

@dtantsur
Copy link
Owner

dtantsur commented Aug 1, 2024

Took a deeper look. I guess "volume" should be called "block-storage" now, so it's actually a bug. "identity" is reserved for the future, so maybe it's fair to exclude it in the linter, but also it may be better to just add it.

@gierens gierens force-pushed the fix-cfg-linting branch 5 times, most recently from ce3e149 to aced2c6 Compare August 2, 2024 08:52
@gierens gierens changed the title Fix Rust 1.80 cfg Linting Fix Rust 1.80 cfg Linting and add VolumeRef impls Aug 2, 2024
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
…atures

Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
@gierens
Copy link
Contributor Author

gierens commented Aug 2, 2024

Took a deeper look. I guess "volume" should be called "block-storage" now, so it's actually a bug. "identity" is reserved for the future, so maybe it's fair to exclude it in the linter, but also it may be better to just add it.

@dtantsur Yeah you're right, I should've looked into the code causing the issue ... I adjusted the PR accordingly:

  • I added the identity feature with a comment that it's reserved for future use.
  • The VolumeRef feature was changed to block-storage and I added the missing impls in the block-storage module.
  • For SnapshotRef this is not gonna work without a snapshot API though, so I simply ignored it in the linting for now, but added TODO comments that this is going to get the block-storage feature once snapshots have been implemented. This is actually next on my list after the CI is fixed, so should really just be a temporary thing.
  • The test feature which is used in some tests I also added to the linting rules, since this is not a real feature.

@gierens
Copy link
Contributor Author

gierens commented Aug 2, 2024

@dtantsur Alright, CI seems to approve. So this is ready for another review.

@dtantsur dtantsur merged commit 508cb6c into dtantsur:master Aug 2, 2024
16 checks passed
@dtantsur
Copy link
Owner

dtantsur commented Aug 2, 2024

Thanks much!

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.

2 participants