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

Add a basic rust-toolchain.toml #176

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sellout
Copy link
Collaborator

@sellout sellout commented Dec 6, 2024

Matches the one in Zebra, although I think pinning to a specific release (e.g., channel = "1.82") would help with consistency, perhaps avoiding issues like
#174 (comment)

Matches the one in Zebra, although I think pinning to a specific release
(e.g., `channel = "1.82"`) would help with consistency, perhaps avoiding
issues like
ZcashFoundation#174 (comment)
@sellout
Copy link
Collaborator Author

sellout commented Dec 6, 2024

Ah, good. This captures that the build failures are due to rustc 1.81, and not anything in #174. And also a reason to pin to a specific rustc version – changes outside of the repo shouldn’t cause a working repo to start failing.

@sellout sellout marked this pull request as draft December 6, 2024 21:11
@sellout sellout force-pushed the rust-toolchain branch 4 times, most recently from a07c8e7 to c1eb1e4 Compare December 6, 2024 21:51
[actions-rs/toolchain doesn’t support TOML-formatted
rust-toolchain files](actions-rs/toolchain#126), but it’s unnecessary anyway.

- actions-rs/cargo will pick up the rust-toolchain.toml, so we usually
  don’t need to mention the toolchain at all;
- the Windows build just runs `rustup target add x86_64-pc-windows-msvc`
  directly; and
- where we want to build with multiple toolchains (in a matrix), there
  are some slightly-awkward conditionals.

This also makes some other changes:

- `fail-fast` is disabled because it hides useful & distinct build
  results; and
-  `rustup component add` for clippy and rustfmt are removed because
  they’re in the rust-toolchain.toml toolchain, and we want to make sure
  they are, so that they’re available to developers.
@sellout
Copy link
Collaborator Author

sellout commented Dec 6, 2024

@conradoplg The issue from #174 (comment) occurs on master, this PR tries to pin down rustc a bit to avoid it.

  • 1.81 – works
  • 1.82–1.84 (beta) – not working
  • 1.85 (nightly) – working again

So, the problem seems to be fixed with just a bit of waiting … however I can’t find anything obvious in the 1.82 or 1.85 changelogs that would indicate why this started/stopped happening.

I’ve currently pinned this to 1.81 (and I would ideally pin to a specific version even if there wasn’t a bug, because it’s better for consistency). Zebra would need to be similarly pinned for it to depend on this before mid-Feb.

However (as you can see in the failing check) unsafe extern "C" wasn’t supported until 1.82, and cargo package is complaining about it. But I don’t understand why master passed the same check when it used 1.81 (and there are no changes related to that code in this PR).

I think everything would pass if I pinned to 1.85, but

  1. I can’t actually pin to "1.85" since it’s not released yet, and
  2. I don’t want to pin to "nightly" because it’ll keep shifting.

Maybe I can pin to today’s date?

@sellout sellout force-pushed the rust-toolchain branch 2 times, most recently from fcf210a to 8c332fc Compare December 9, 2024 22:21
Newer versions until 1.85 (current nightly) have some breakage wrt C++
linking.
It should match the version in rust-toolchain.toml. Unfortunately, it’s
not possible to reference that directly, so this adds comments to remind
contributors to update them together.
@sellout
Copy link
Collaborator Author

sellout commented Dec 9, 2024

However (as you can see in the failing check) unsafe extern "C" wasn’t supported until 1.82, and cargo package is complaining about it. But I don’t understand why master passed the same check when it used 1.81 (and there are no changes related to that code in this PR).

I understand why this failure is happening. The unsafe extern is generated by bindgen, which targets latest stable by default. When 1.81 was current, it would have generated extern instead of unsafe extern. Simply adding a rust-toolchain.toml doesn’t affect bindgen, so it was generating 1.83-compatible code, which didn’t work with our rust-toolchain.toml pinning to 1.81.

What’s confusing me now is that the Cargo.lock file pins a version of bindgen (0.69.4) that is too old to generate rustc 1.82-specific code, but one of the CI jobs (“Package (rust_toolchain.toml)”) appears to ignore the Cargo.toml and pull a newer version of bindgen that does support 1.82, which is why that was the only job that was failing.

For now, I am setting the rust_target to the newest version supported by bindgen 0.69.4, which is 1.73, and quieting the deprecation error that the one CI job triggers. But I would love to have that CI job not get a different bindgen version than everything else …

@sellout sellout marked this pull request as ready for review December 9, 2024 22:47
Copy link
Collaborator

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK f6857f1

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.

2 participants