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.

Sign up for GitHub

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

Jump to bottom

Upgrade to indexmap v2 (MSRV 1.63) #741

Merged
merged 2 commits into from
Jul 20, 2024
Merged

Upgrade to indexmap v2 (MSRV 1.63) #741

merged 2 commits into from
Jul 20, 2024

Conversation

cuviper
Copy link
Contributor

@cuviper cuviper commented Jun 27, 2023

No description provided.

cuviper
cuviper commented Jun 27, 2023
View reviewed changes
tower/src/ready_cache/cache.rs
@@ -193,7 +193,7 @@ where
}

/// Obtains a mutable reference to a service in the ready set by index.
pub fn get_ready_index_mut(&mut self, idx: usize) -> Option<(&mut K, &mut S)> {
pub fn get_ready_index_mut(&mut self, idx: usize) -> Option<(&K, &mut S)> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a breaking change, so I suppose it should go on your 0.5 milestone. But if you really want the mutable key, there's an opt-in by using the MutableKeys trait and its get_index_mut2 method.

Copy link
Member

Choose a reason for hiding this comment

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

It's feels like more of a footgun to have the key be mutable, given the warnings mentioned in MutableKeys::get_index_mut2... and there's really nothing in the original PR or docs that speaks specifically to the key needing to be mutable... so I think this is a worthwhile change to make given that changing the MSRV is breaking anyways.

@cuviper cuviper changed the title Upgrade to indexmap v2 (MSRV 1.64) Upgrade to indexmap v2 (MSRV 1.63) Nov 28, 2023
@cuviper
Copy link
Contributor Author

cuviper commented Nov 28, 2023

We were later able to relax hashbrown and indexmap to MSRV 1.63, so I've updated that here. Either way, this is well beyond tower's stated 6-month MSRV policy.

This was referenced Nov 28, 2023
Closed
Closed
@serprex serprex mentioned this pull request Dec 27, 2023
Closed
@Ameobea
Copy link

Ameobea commented Feb 7, 2024

ahash, a dependency of v1 indexmap, no longer compiles on new Rust nightly versions: tkaitchuck/aHash#200

As a result, the latest version of tower no longer builds on Rust nightly

This PR should fix that problem

@cuviper
Copy link
Contributor Author

cuviper commented Feb 7, 2024

ahash, a dependency of v1 indexmap

I'm not sure what you're seeing, but indexmap has never had a dependency on ahash. Even its hashbrown dependency has always been with default-features = false to avoid ahash there, unless something else enabled that.

@Ameobea
Copy link

Ameobea commented Feb 8, 2024

ahash, a dependency of v1 indexmap

I'm not sure what you're seeing, but indexmap has never had a dependency on ahash. Even its hashbrown dependency has always been with default-features = false to avoid ahash there, unless something else enabled that.

You're right, I wasn't precise. ahash is a dependency on more level down as you pointed out: tower -> indexmap -> hashbrown -> ahash.

Pulling in tower by itself with no other crates, even with features = ["full"] set, still compiles on nightly because hashbrown has default-features = false set by default.

It seems some other crate in our workspace is turning on the ahash feature of hashbrown. The output of cargo tree was showing ahash in the tree underneath tower even though the feature got turned on elsewhere.

So in that case, bumping indexmap to v2 on tower probably wouldn't help out since there is some other crate somewhere depending on it and turning on the feature.

Apologies for the confusion here.

@cuviper
Copy link
Contributor Author

cuviper commented Feb 8, 2024

OK -- cargo tree -i ahash -e features may help you track that down!

tobz
tobz approved these changes Jul 20, 2024
View reviewed changes
tower/src/ready_cache/cache.rs
@@ -193,7 +193,7 @@ where
}

/// Obtains a mutable reference to a service in the ready set by index.
pub fn get_ready_index_mut(&mut self, idx: usize) -> Option<(&mut K, &mut S)> {
pub fn get_ready_index_mut(&mut self, idx: usize) -> Option<(&K, &mut S)> {
Copy link
Member

Choose a reason for hiding this comment

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

It's feels like more of a footgun to have the key be mutable, given the warnings mentioned in MutableKeys::get_index_mut2... and there's really nothing in the original PR or docs that speaks specifically to the key needing to be mutable... so I think this is a worthwhile change to make given that changing the MSRV is breaking anyways.

@tobz tobz added A-ready-cache Area: The tower "ready_cache" middleware C-cleanup Category: PRs that clean code up or issues documenting cleanup. relnotes Marks issues that should be documented in the release notes of the next release. labels Jul 20, 2024
@tobz tobz mentioned this pull request Jul 20, 2024
Closed
@tobz tobz merged commit 05a0a25 into tower-rs:master Jul 20, 2024
14 checks passed
@tobz tobz added the S-awaiting-release Status: approved/merged but awaiting a release. label Jul 20, 2024
@tobz tobz mentioned this pull request Jul 20, 2024
Closed
@tobz
Copy link
Member

tobz commented Aug 13, 2024

This has been released as part of [email protected].

Thanks again for your contribution!

@tobz tobz removed the S-awaiting-release Status: approved/merged but awaiting a release. label Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobz tobz tobz approved these changes

Assignees
No one assigned
Labels
A-ready-cache Area: The tower "ready_cache" middleware C-cleanup Category: PRs that clean code up or issues documenting cleanup. relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cuviper @Ameobea @tobz