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

ci(stable): fix build issues to prepare for v1.28.0 release #4105

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

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented Dec 4, 2024

Before merging, we should also:


FWIW, because of rustls's migration to aws-lc-rs, with this PR:

  • The reqwest/rustls/aws-lc-rs combo has become the default download backend on Linux for riscv*, s390x* and ppc32 platforms.
  • netbsd and illumos builds have switched to reqwest/rustls/native-tls by default.

@rami3l rami3l added this to the 1.28.0 milestone Dec 4, 2024
@rami3l rami3l force-pushed the ci/1-28-0 branch 2 times, most recently from 355a1b2 to 18bda7c Compare December 4, 2024 06:32
@heiher
Copy link
Contributor

heiher commented Dec 4, 2024

We need to temporarily disable the rustls feature for loongarch64-linux-{gnu,musl} until the libclang-dev adds support the LoongArch target.

rustup/ci/run.bash

Lines 17 to 25 in 14470b2

case "$TARGET" in
# these platforms aren't supported by ring:
powerpc* ) ;;
mips* ) ;;
riscv* ) ;;
s390x* ) ;;
# default case, build with rustls enabled
* ) FEATURES+=('--features' 'reqwest-rustls-tls') ;;
esac

rustup/ci/run.bash

Lines 42 to 50 in 14470b2

case "$TARGET" in
# these platforms aren't supported by ring:
powerpc* ) ;;
mips* ) ;;
riscv* ) ;;
s390x* ) ;;
# default case, build with rustls enabled
* ) features+=('--features' 'reqwest-rustls-tls') ;;
esac

@rami3l
Copy link
Member Author

rami3l commented Dec 4, 2024

We need to temporarily disable the rustls feature for loongarch64-linux-{gnu,musl} until the libclang-dev adds support the LoongArch target.

@heiher Thanks for the heads up! I guess removing reqwest-rustls-tls would be enough as a last resort (#4094 (comment)). In the meantime, I'll try to configure bindgen-cli for the rest of the failures.

@heiher
Copy link
Contributor

heiher commented Dec 5, 2024

I tested the rustup-init-loongarch64-unknown-linux-gnu, and the following error was thrown when downloading:

error: could not download file from 'https://static.rust-lang.org/dist/channel-rust-stable.toml.sha256' to '/home/hev/.rustup/tmp/nf3c6b3n2mqj5w73_file': failed to make network request: download backend 'reqwest rustls' unavailable

@rami3l
Copy link
Member Author

rami3l commented Dec 5, 2024

I tested the rustup-init-loongarch64-unknown-linux-gnu, and the following error was thrown when downloading:

error: could not download file from 'https://static.rust-lang.org/dist/channel-rust-stable.toml.sha256' to '/home/hev/.rustup/tmp/nf3c6b3n2mqj5w73_file': failed to make network request: download backend 'reqwest rustls' unavailable

@heiher Despite being not idiomatic, that's the expected behavior currently, as reqwest-rustls-tls is the new default now, so you'll need to export RUSTUP_USE_RUSTLS=0:

rustup/src/utils/mod.rs

Lines 251 to 253 in 14470b2

let use_rustls = process
.var_os("RUSTUP_USE_RUSTLS")
.is_none_or(|it| it != "0");

... it'd definitely be nice to add cfg!() to the predicate before actually shipping this build, I believe.

@heiher
Copy link
Contributor

heiher commented Dec 5, 2024

I tested the rustup-init-loongarch64-unknown-linux-gnu, and the following error was thrown when downloading:

error: could not download file from 'https://static.rust-lang.org/dist/channel-rust-stable.toml.sha256' to '/home/hev/.rustup/tmp/nf3c6b3n2mqj5w73_file': failed to make network request: download backend 'reqwest rustls' unavailable

@heiher Despite being not idiomatic, that's the expected behavior currently, as reqwest-rustls-tls is the new default now, so you'll need to export RUSTUP_USE_RUSTLS=0:

rustup/src/utils/mod.rs

Lines 251 to 253 in 14470b2

let use_rustls = process
.var_os("RUSTUP_USE_RUSTLS")
.is_none_or(|it| it != "0");

... it'd definitely be nice to add cfg!() to the predicate before actually shipping this build, I believe.

@rami3l Thank you. Could you include this patch together as well?

diff --git a/src/utils/mod.rs b/src/utils/mod.rs
index dae45d4b..d9a8ee51 100644
--- a/src/utils/mod.rs
+++ b/src/utils/mod.rs
@@ -250,7 +250,8 @@ async fn download_file_(
         .is_some_and(|it| it != "0");
     let use_rustls = process
         .var_os("RUSTUP_USE_RUSTLS")
-        .is_none_or(|it| it != "0");
+        .is_none_or(|it| it != "0")
+        && cfg!(feature = "reqwest-rustls-tls");
     let (backend, notification) = if use_curl_backend {
         (Backend::Curl, Notification::UsingCurl)
     } else {

@rami3l
Copy link
Member Author

rami3l commented Dec 5, 2024

@heiher Done! I guess it should be working now?

@heiher
Copy link
Contributor

heiher commented Dec 5, 2024

@heiher Done! I guess it should be working now?

Acked. 👍

@rami3l
Copy link
Member Author

rami3l commented Dec 5, 2024

Blocking on aws/aws-lc-rs#620.

@rami3l rami3l marked this pull request as draft December 7, 2024 01:13
@rami3l
Copy link
Member Author

rami3l commented Dec 7, 2024

Oops, it looks like https://github.com/rust-lang/rust-bindgen/releases/tag/v0.71.0 has suddenly broken everything...

Not only are the prebuilt binaries and installer missing (as reported in rust-lang/rust-bindgen#3038, though our fallback to cargo install did work perfectly), but all bindgen-cli related workflows, namely those that depend on aws-lc, are now failing.

@rami3l
Copy link
Member Author

rami3l commented Dec 7, 2024

I could lock the bindgen-cli version back to the old one, though I do think it's better to figure out what happened first.

@rami3l
Copy link
Member Author

rami3l commented Dec 8, 2024

I strongly suspect that rust-lang/rust-bindgen#3039 has caused the build failure; checking --version upfront seems to be pretty standard WRT calling any subprocess.

Update: theory confirmed (https://github.com/aws/aws-lc-rs/blob/a7a70cbe163b22f6a4cd438ad2d0c668e2fbdc58/aws-lc-sys/builder/main.rs#L700).

@rami3l rami3l force-pushed the ci/1-28-0 branch 2 times, most recently from 78d77d0 to bfdc2fb Compare December 9, 2024 12:56
@rami3l rami3l marked this pull request as ready for review December 9, 2024 12:58
@rami3l
Copy link
Member Author

rami3l commented Dec 11, 2024

As aws/aws-lc-rs#627 is now open, I'll request the team's review first.

Of course, I'll finish the checklist before actually merging this PR.

@rami3l rami3l requested review from djc and ChrisDenton December 11, 2024 12:56
@ChrisDenton
Copy link
Member

I'm not yet familiar with rustup's CI setup. Is skip-master kept because we still expect master to fail after this PR is merged?

@rami3l
Copy link
Member Author

rami3l commented Dec 11, 2024

I'm not yet familiar with rustup's CI setup. Is skip-master kept because we still expect master to fail after this PR is merged?

@ChrisDenton We don't run everything on master (that's how things had always been done way before my CI refactoring last year), and essentially the only moment we need to make sure all workflows are green is during a release, which is related to the stable CI rather than the master one.

I've already triggered the stable CI last week, and it failed; according to the parts that have failed during that run, I have temporarily enabled some workflows just for this PR. They will be disabled again before this PR is merged.

@rami3l
Copy link
Member Author

rami3l commented Dec 12, 2024

I'm not yet familiar with rustup's CI setup. Is skip-master kept because we still expect master to fail after this PR is merged?

@ChrisDenton BTW if you're wondering why the master CI is currently failing, that's not related to the build-time behavior, but rather aws-cli, so essentially not an issue on our side, unfortunately :(

Update: I've made #4111.

@rami3l
Copy link
Member Author

rami3l commented Dec 18, 2024

@djc aws-lc-* deps have been updated and are verified to build in https://github.com/rust-lang/rustup/actions/runs/12384929706 (don't mind the illumos build failure, I've now switched aws-lc-rs off for it while rebasing). As such, this PR is ready to be merged.

Cargo.lock Show resolved Hide resolved
TlsBackend::Rustls
} else {
#[cfg(feature = "reqwest-native-tls")]
#[cfg(feature = "reqwest-rustls-tls")]
Copy link
Contributor

Choose a reason for hiding this comment

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

See #4118.

Copy link
Member Author

@rami3l rami3l Dec 19, 2024

Choose a reason for hiding this comment

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

@djc Acknowledged. Blocking on #4118.

Copy link
Member Author

@rami3l rami3l Dec 19, 2024

Choose a reason for hiding this comment

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

@djc Wait, if I drop c4f166e, this PR will be functionally orthogonal with #4118, so it doesn't really have to block on #4118; on the contrary we'll just have to work in parallel (you on #4118 and I on #4105 (comment)) and call it a day?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, makes sense!

@@ -1,4 +1,8 @@
FROM rust-x86_64-unknown-freebsd

# Building `aws-lc-rs` for Linux depends on `gcc-multilib`, `libclang` and `bindgen`.
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels confusing to me that we mention "Linux" in these FreeBSD/Illumos/NetBSD Dockerfiles. Maybe use the proper target OS name in the first line?

Should also suggest upstream that they clarify this page in their documentation that it applies to other Unix-likes too, ideally.

Copy link
Member Author

Choose a reason for hiding this comment

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

@djc You're right. This info was extracted from their doc page and probably that very page needs to be updated.

ci/run.bash Show resolved Hide resolved
@djc
Copy link
Contributor

djc commented Dec 18, 2024

Thanks for all the work on this so far!

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.

4 participants