Use portable-atomic for targets which lack 64-bit atomics used to check interpreter ID.

[adamreichold]

I chose to make the dependency mandatory instead of optional as portable-atomic itself just forwards to the native atomics when they are available so making that choice part of our build system is not really necessary. Personally, I was unable to perceive any noticeable compile-time hit from adding it.

Closes #3614

[codspeed-hq]

CodSpeed Performance Report

Merging #3619 will improve performances by 22.03%

_{Comparing portable-atomic (e6457c5) with main (81ad2e8)}

Summary

⚡ 1 improvements
✅ 77 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	portable-atomic	Change
⚡	list_via_downcast	153.9 ns	126.1 ns	+22.03%

[davidhewitt]

Given this is a single dependency with no mandatory dependencies of its own I agree there's a good argument to having this be mandatory.

@alex I know you are keen to minimise dependencies, I think in this case it makes sense to add this one though?

[alex]

Would it make sense for this to be a platform-specific dep?

[target.'cfg(whatever condition)'.dependencies]
portable-atomics = "1"

If that's impractical, I understand.

[birkenfeld]

Is it really worth that complication for such a tiny dependency?

Also, if we code that condition specific for the currently problematic platform, who's to say that there won't be another soon?

I'd say, leave the cfg-ing in one spot, i.e. that crate itself.

[alex]

It's a bit unfortunate as portable-atomics relies on a build script to accomplish it's platform detection. Build scripts can disproportionately contribute to compilation time.

But as I said, if doing the conditional dependency is impractical, I understand.

[alex]

Can we use not(cfg(target_has_atomic = "64")) here? https://doc.rust-lang.org/reference/conditional-compilation.html#target_has_atomic

[adamreichold]

Can we use not(cfg(target_has_atomic = "64")) here? https://doc.rust-lang.org/reference/conditional-compilation.html#target_has_atomic

I think that particular flag is out of scope for now due to requiring Rust 1.60, c.f. https://github.com/rust-lang/rust/blob/master/RELEASES.md#language-14 (I am also not sure if it works inside Cargo.toml instead of during compilation.)

I am not against making this a conditional dependency but I would add that this somehow goes against portable-atomics reason d'etre as a polyfill. I would even guess that it needs the build script exactly because it detects the presence of atomics without cfg(target_has_atomic), probably among other things. So by explicitly using it for some platforms we are basically manually re-doing its platform detection logic in an incomplete manner.

Hence, if we really want a conditional dependency, I would prefer to just make it optional so that downstreams can enable it as a feature to get maximum portability. Or maybe even add it as a default feature so that downstream can opt out of the build time hit and the reproducibility issues generally associated with build scripts.

So what do you think? Optional or not? Enabled by default or not?

[alex]

What do you think about just making it mandatory (i.e., leaving this PR as-is) for now, and then migrating to the cfg() support when we raise our MSRV?

[adamreichold]

What do you think about just making it mandatory (i.e., leaving this PR as-is) for now, and then migrating to the cfg() support when we raise our MSRV?

Yes, we can certainly do that as well. I also checked and

[target.'cfg(not(target_has_atomic = "64"))'.dependencies]
portable-atomic = "1.0"

does work as expected on a current toolchain.

[alex]

That sounds good to me -- I suspect we can bump to 1.60 or even 1.63 very very soon :-)

[davidhewitt]

Looks good to me to merge this then, thanks everyone for figuring out a great way forward 👍

[mgorny]

Would it be possible to have this backported to 0.20.x?

[davidhewitt]

Depends, @mgorny do you consider adding an additional dependency for a bugfix to be semver breaking? I deliberately skipped this out of caution, as my opinion was yes. As you're a packaging expert I'd be happy to listen to your opinion, especially as I agree this fix could be valuable.

[davidhewitt]

(I also had hoped to have 0.21 released months ago 👀)

[mgorny]

Depends, @mgorny do you consider adding an additional dependency for a bugfix to be semver breaking? I deliberately skipped this out of caution, as my opinion was yes. As you're a packaging expert I'd be happy to listen to your opinion, especially as I agree this fix could be valuable.

Well, I suppose it's a bit of a gray area. In general, I wouldn't consider it to be breaking, presuming that adding a new dependency doesn't break consumers. I'm not an expert on Rust but I think that the risk of breakage is minimal here.

Worst case, you can revert and make another release, right? ;-)

[davidhewitt]

Sure thing, in which case I'll aim to look into shipping a patch release with both this and #3555 for 0.20 in the coming days.

[mgorny]

Thanks a lot! Hopefully we can get all the projects to update soonish and unblock the ppc32 queue on Gentoo.

[davidhewitt]

Sorry @mgorny this is not forgotten just had other stuff keep coming up and sickness in the family, we're now better though.

I'm hoping to put this together over the weekend but I guess my past promise doesn't leave much faith in this hope either 🙈

[mgorny]

Gentle ping. This is now blocking us from upgrading cryptography to a non-vulnerable version.

[davidhewitt]

😞 This has taken longer than I wanted. Ideally I can get a final bugfix #3834 through review by tonight and then ship the combined patch release tomorrow or Friday. Please forgive the delay here; the work on the upcoming PyO3 0.21 has been consuming me (and that release is hopefully less than a couple weeks away too).