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

Conditionally compile in Arc #140

Merged
merged 1 commit into from Nov 5, 2022
Merged

Conversation

CryZe
Copy link
Contributor

@CryZe CryZe commented Nov 4, 2022

Apparently the latest release of bytemuck doesn't compile on some of the targets that it used to because it uses Arc. Arc is not a type that exists on every target, because not every target supports atomics.

Error message with 1.12.2:

error[E0432]: unresolved import `alloc::sync`
  --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/bytemuck-1.12.2/src/allocation.rs:17:3
   |
17 |   sync::Arc,
   |   ^^^^ could not find `sync` in `alloc`

Apparently the latest release of `bytemuck` doesn't compile on some of
the targets that it used to because it uses `Arc`. `Arc` is not a type
that exists on every target, because not every targets supports atomics.
Copy link
Contributor

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but wasn't target_has_atomic introduced in a Rust version after 1.34? This would break the MSRV.

@CryZe
Copy link
Contributor Author

CryZe commented Nov 4, 2022

The extern_crate_alloc feature does not follow the MSRV. At least that's how I'm reading the README (and how the CI is set up). Alternatively arc could be a feature (that internally also does the check) and 1.12.2 gets yanked.

Opt-in features of the crate are not held to the same standard, and may work only on the latest Stable or latest Nightly.

@Lokathor
Copy link
Owner

Lokathor commented Nov 5, 2022

That's accurate, the CI isn't that precise with the MSRV. Only the "basic" build sticks exactly to 1.34. Everything else is only tested on latest stable.

I'd rather not create even more features for people to have to manage, if it's reasonable to just keep two features together. In the case of target_has_atomic, it seems to be the "intended' cfg to check for this sort of thing. According to rust-lang/rust#32976 it's been stable since 1.60 back in Feb, which seems "long enough" that we can generally assume people are using at least that version. That's new enough that it's not in Debian Stable, but it is in Debian Testing, which should become the stable version sometime in 2023.

I'm inclined to accept this as is, unless @notgull you really think we should break it up?

@notgull
Copy link
Contributor

notgull commented Nov 5, 2022

I'm fine with this! However, I do think that an MSRV for the alloc feature (and others) should be defined to prevent surprises.

@Lokathor
Copy link
Owner

Lokathor commented Nov 5, 2022

I agree, but I'm not sure I have the energy for that much CI fiddling

@Lokathor Lokathor merged commit 02021fb into Lokathor:main Nov 5, 2022
@CryZe CryZe deleted the fix-arc-compile-error branch November 5, 2022 13:01
@CryZe
Copy link
Contributor Author

CryZe commented Nov 5, 2022

Can we have a bugfix release for this? My CI blocks me otherwise xD

@Lokathor
Copy link
Owner

Lokathor commented Nov 5, 2022

Yeah, I had enough time to hit the button before the bus to work but can't sit down at a publish capable thing until some time later today.

@CryZe
Copy link
Contributor Author

CryZe commented Nov 5, 2022

Ah no worries then :D

@Lokathor
Copy link
Owner

Lokathor commented Nov 5, 2022

1.12.3 should be available now

@CryZe
Copy link
Contributor Author

CryZe commented Nov 5, 2022

Nice, everything works now 👍
Thank you :)

@Lokathor Lokathor added the semver-patch semver patch change label Jan 20, 2023
leod pushed a commit to leod/bytemuck that referenced this pull request Jun 3, 2023
Apparently the latest release of `bytemuck` doesn't compile on some of
the targets that it used to because it uses `Arc`. `Arc` is not a type
that exists on every target, because not every targets supports atomics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch semver patch change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants