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

Use portable-atomic when AtomicU64 is not available. #233

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

flaviojs
Copy link
Contributor

@flaviojs flaviojs commented Jan 7, 2025

edit Only fixes #232 if applied to a 0.8.x branch or if opendal moves to 0.9.x.

AtomicU64 was introduced in v0.8.4
The current version is 0.9.0, please update 0.8.x too.

@djc
Copy link
Owner

djc commented Jan 7, 2025

Why do you need support for 0.8.x?

@flaviojs
Copy link
Contributor Author

flaviojs commented Jan 7, 2025

As stated in the issue, I am trying to compile sccache.
According to cargo tree, sccache depends on opendal, which depends on bb8.

OpenDAL has this in their Cargo.toml:

bb8 = { version = "0.8", optional = true }

Due to semver rules, the most recent v0.8.x version compatible with all dependencies will be pulled in.
Starting with v0.8.4 I am unable to compile bb8 due to missing AtomicU64, so I need a new v0.8.x version with this fix.

@djc
Copy link
Owner

djc commented Jan 7, 2025

Maybe get OpenDAL to update to bb8 0.9 instead? As a single volunteer maintainer I don't have much bandwidth for maintaining older versions.

@flaviojs
Copy link
Contributor Author

flaviojs commented Jan 7, 2025

According to https://crates.io/crates/bb8/reverse_dependencies a lot more crates would need updating...

This can be though of as a bug introduced in 0.8.4 that affects any system that does not have AtomicU64.
Ideally what needs to happen is:

  • merge this PR, bump master version to 0.9.1, and publish
  • create branch 0.8.x in the v0.8.6 tag. apply this PR to it, bump 0.8.x version to 0.8.7, publish and maybe yank 0.8.4 to 0.8.6 too(?)

I'm willing to do the work, but what I can do depends on the level of permissions I have.
Currently the most I can do is add a version bump to 0.9.1 to this PR, and make a PR against a future 0.8.x branch with this fix and a version bump to 0.8.7.
If you are willing to trust a total stranger and give me whatever permissions are needed to do the rest, then I will do the rest. Note that I've never published a crate before so am not sure what is needed.

@djc
Copy link
Owner

djc commented Jan 7, 2025

I'm definitely not giving a complete stranger a bunch of permissions. If this (arguably a pretty niche issue) is so important to you, let's discuss funding?

bb8/src/internals.rs Outdated Show resolved Hide resolved
@flaviojs
Copy link
Contributor Author

flaviojs commented Jan 7, 2025

Dude, I'm just a random programmer that found a bug, reported it to the appropriate place and provided a fix for it with the suggested dependency. Donating my free time IS the kind of "funding" a person like me provides.

If the funding you require is money then please wait for the unlikely case of someone whose job depends on this crate and who's company policy doesn't involve forking dependencies and fixing things internally... 😞

@djc
Copy link
Owner

djc commented Jan 7, 2025

Dude, I'm just a random programmer that found a bug, reported it to the appropriate place and provided a fix for it with the suggested dependency. Donating my free time IS the kind of "funding" a person like me provides.

If the funding you require is money then please wait for the unlikely case of someone whose job depends on this crate and who's company policy doesn't involve forking dependencies and fixing things internally... 😞

I appreciate that it sucks to be in your position. However, as someone who provides hundreds of hours per month of free labor to a zillion companies, doing backports (for this kind of niche architecture) is where I draw the line.

(And after I pointed you in the right direction and reviewed your changes, I don't think I deserve being "Dude"d at.)

@djc djc merged commit 74519c8 into djc:main Jan 7, 2025
7 of 8 checks passed
@flaviojs
Copy link
Contributor Author

flaviojs commented Jan 7, 2025

I'm glad it got merged. It seems like 0.8.x won't be getting the fix which sucks a lot.
I'll inform opendal of the problem in this crate after you make a 0.9.x release with this fix.

Dude... you interpreted "dude" as something negative?
To avoid misunderstandings: dude is informal, I speak informally and directly pretty much all the time and I don't know of any other appropriate word for what I want to transmit from my native language.
Even if historically the word was bad, so far I have never actually seen a case where it was used in a negative way.

@flaviojs flaviojs deleted the add-portable-atomic branch January 7, 2025 22:42
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.

AtomicU64 unresolved symbol
2 participants