Skip to content

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

Merged
djc merged 2 commits into
djc:mainfrom
flaviojs:add-portable-atomic
Jan 7, 2025
Merged

Use portable-atomic when AtomicU64 is not available.#233
djc merged 2 commits into
djc:mainfrom
flaviojs:add-portable-atomic

Conversation

@flaviojs

@flaviojs flaviojs commented Jan 7, 2025

Copy link
Copy Markdown
Contributor

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

djc commented Jan 7, 2025

Copy link
Copy Markdown
Owner

Why do you need support for 0.8.x?

@flaviojs

flaviojs commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

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

djc commented Jan 7, 2025

Copy link
Copy Markdown
Owner

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

flaviojs commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

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

djc commented Jan 7, 2025

Copy link
Copy Markdown
Owner

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?

Comment thread bb8/src/internals.rs Outdated
@flaviojs

flaviojs commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

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

djc commented Jan 7, 2025

Copy link
Copy Markdown
Owner

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
@flaviojs

flaviojs commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

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