Skip to content

Conditionally compile in Arc#140

Merged
Lokathor merged 1 commit intoLokathor:mainfrom
CryZe:fix-arc-compile-error
Nov 5, 2022
Merged

Conditionally compile in Arc#140
Lokathor merged 1 commit intoLokathor:mainfrom
CryZe:fix-arc-compile-error

Conversation

@CryZe
Copy link
Copy Markdown
Contributor

@CryZe CryZe commented Nov 4, 2022 •
edited
Loading

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`

@CryZe
Conditionally compile in Arc
99ed627 
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.
@CryZe CryZe mentioned this pull request Nov 4, 2022
Merged
notgull
notgull reviewed Nov 4, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

CryZe commented Nov 4, 2022 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Contributor

notgull commented Nov 5, 2022 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Contributor Author

CryZe commented Nov 5, 2022

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

@Lokathor
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

CryZe commented Nov 5, 2022

Ah no worries then :D

@Lokathor
Copy link
Copy Markdown
Owner

Lokathor commented Nov 5, 2022

1.12.3 should be available now

@CryZe
Copy link
Copy Markdown
Contributor Author

CryZe commented Nov 5, 2022

Nice, everything works now 👍
Thank you :)

@h3r2tic h3r2tic mentioned this pull request Nov 7, 2022
Merged
@MarijnS95 MarijnS95 mentioned this pull request Nov 7, 2022
Merged
@Lokathor Lokathor mentioned this pull request Dec 20, 2022
Merged
@Lokathor Lokathor added the semver-patch semver patch change label Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@notgull notgull notgull left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver-patch semver patch change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@CryZe @Lokathor @notgull