Skip to content

Commit

Permalink
chore(turbopack): Centralize reqwest TLS feature configs in turbo-tas…
Browse files Browse the repository at this point in the history
…ks-fetch (vercel#72526)

Noticed this mess while working with @samcx on vercel#72442 where we had to run:

```
cargo nextest r --features next-core/native-tls -p next-core
```

This PR makes `turbo-tasks-fetch` responsible for setting the `reqwest` TLS features, rather than trying to pass that feature flag down.

After this PR, running tests on `next-core` works with just:

```
cargo nextest r -p next-core
```

## Why?

- The feature flag logic was getting duplicated in multiple top-level targets.
- Packages that depended on `turbo-tasks-fetch` (directly *or transitively*) had to specify a default feature and make sure that default feature was disabled in the workspace's dependency list so that `cargo test -p package-name` would work
- ... or you had to specify a flag when compiling, like `--features next-core/native-tls`.

Technically this change gives us less flexibility. We can't produce multiple binaries for the same platform target with different TLS stacks. But I don't think we would've ever used that flexibility anyways.
  • Loading branch information
bgw authored Nov 14, 2024
1 parent 0bacb05 commit 57e732f
Show file tree
Hide file tree
Showing 11 changed files with 26 additions and 73 deletions.
10 changes: 2 additions & 8 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ turbo-tasks-backend = { path = "turbopack/crates/turbo-tasks-backend" }
turbo-tasks-build = { path = "turbopack/crates/turbo-tasks-build" }
turbo-tasks-bytes = { path = "turbopack/crates/turbo-tasks-bytes" }
turbo-tasks-env = { path = "turbopack/crates/turbo-tasks-env" }
turbo-tasks-fetch = { path = "turbopack/crates/turbo-tasks-fetch", default-features = false }
turbo-tasks-fetch = { path = "turbopack/crates/turbo-tasks-fetch" }
turbo-tasks-fs = { path = "turbopack/crates/turbo-tasks-fs" }
turbo-tasks-hash = { path = "turbopack/crates/turbo-tasks-hash" }
turbo-tasks-macros = { path = "turbopack/crates/turbo-tasks-macros" }
Expand Down Expand Up @@ -102,13 +102,6 @@ swc_emotion = { version = "0.72.28" }
swc_relay = { version = "0.44.30" }

# General Deps

# Be careful when selecting tls backend, including change default tls backend.
# If you changed, must verify with ALL build targets with next-swc to ensure
# it works. next-swc have various platforms, some doesn't support native (using openssl-sys)
# and some aren't buildable with rustls.
reqwest = { version = "=0.11.17", default-features = false }

chromiumoxide = { version = "0.5.4", features = [
"tokio-runtime",
], default-features = false }
Expand Down Expand Up @@ -180,6 +173,7 @@ quote = "1.0.23"
rand = "0.8.5"
rayon = "1.10.0"
regex = "1.10.6"
reqwest = { version = "=0.11.17", default-features = false }
rstest = "0.16.0"
rustc-hash = "1.1.0"
semver = "1.0.16"
Expand Down
7 changes: 0 additions & 7 deletions crates/napi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,6 @@ __internal_dhat-heap = ["dhat"]
# effectively does nothing.
__internal_dhat-ad-hoc = ["dhat"]

# Enable specific tls features per-target.
[target.'cfg(all(target_os = "windows", target_arch = "aarch64"))'.dependencies]
next-core = { workspace = true, features = ["native-tls"] }

[target.'cfg(not(any(all(target_os = "windows", target_arch = "aarch64"), target_arch="wasm32")))'.dependencies]
next-core = { workspace = true, features = ["rustls-tls"] }

[lints]
workspace = true

Expand Down
8 changes: 1 addition & 7 deletions crates/next-build-test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ workspace = true
anyhow = { workspace = true }
futures-util = "0.3.30"
next-api = { workspace = true }
next-core = { workspace = true }
num_cpus = "1.16.0"
rand = { workspace = true, features = ["small_rng"] }
serde_json = { workspace = true }
Expand All @@ -36,12 +37,5 @@ turbopack-node = { workspace = true }
turbopack-nodejs = { workspace = true }
turbopack-trace-utils = { workspace = true }

# Enable specific tls features per-target.
[target.'cfg(all(target_os = "windows", target_arch = "aarch64"))'.dependencies]
next-core = { workspace = true, features = ["native-tls"] }

[target.'cfg(not(any(all(target_os = "windows", target_arch = "aarch64"), target_arch="wasm32")))'.dependencies]
next-core = { workspace = true, features = ["rustls-tls"] }

[build-dependencies]
turbo-tasks-build = { workspace = true }
2 changes: 0 additions & 2 deletions crates/next-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@ turbo-tasks-build = { workspace = true }

[features]
next-font-local = []
native-tls = ["turbo-tasks-fetch/native-tls"]
rustls-tls = ["turbo-tasks-fetch/rustls-tls"]
plugin = [
"swc_core/plugin_transform_host_native",
"turbopack-ecmascript-plugins/swc_ecma_transform_plugin",
Expand Down
6 changes: 0 additions & 6 deletions crates/next-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,3 @@ pub fn register() {
turbopack_ecmascript_plugins::register();
include!(concat!(env!("OUT_DIR"), "/register.rs"));
}

#[cfg(all(feature = "native-tls", feature = "rustls-tls"))]
compile_error!("You can't enable both `native-tls` and `rustls-tls`");

#[cfg(all(not(feature = "native-tls"), not(feature = "rustls-tls")))]
compile_error!("You have to enable one of the TLS backends: `native-tls` or `rustls-tls`");
11 changes: 5 additions & 6 deletions packages/next-swc/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,15 @@ pnpm build-wasm
Due to platform differences napi bindings selectively enables supported features.
See below tables for the currently enabled features.

| arch\platform | Linux(gnu) | Linux(musl) | Darwin | Win32 |
| arch\platform | Linux(gnu) | Linux(musl) | Darwin | Windows |
| ------------- | ---------- | ----------- | --------- | --------- |
| ia32 | | | | a,b,d,e |
| x64 | a,b,d,e,f | a,b,d,e,f | a,b,d,e,f | a,b,d,e,f |
| aarch64 | a,d,e,f | a,d,e,f | a,b,d,e,f | a,b,c,e |

- a: `turbo_tasks_malloc`,
- b: `turbo_tasks_malloc_custom_allocator`,
- c: `native-tls`,
- d: `rustls-tls`,
- a: `turbo_tasks_malloc`
- b: `turbo_tasks_malloc_custom_allocator`
- c: `native-tls` (via `turbo-tasks-fetch`)
- d: `rustls-tls` (via `turbo-tasks-fetch`)
- e: `image-extended` (webp)
- f: `plugin`

Expand Down
4 changes: 2 additions & 2 deletions packages/next-swc/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"cache-build-native": "[ -d native ] && echo $(ls native)",
"rust-check-fmt": "cd ../..; cargo fmt -- --check",
"rust-check-clippy": "cargo clippy --workspace --all-targets -- -D warnings -A deprecated",
"rust-check-napi-rustls": "cargo check -p next-swc-napi",
"test-cargo-unit": "cargo nextest run --workspace --exclude next-swc-napi --release --no-fail-fast --features rustls-tls"
"rust-check-napi": "cargo check -p next-swc-napi",
"test-cargo-unit": "cargo nextest run --workspace --exclude next-swc-napi --release --no-fail-fast"
},
"napi": {
"name": "next-swc",
Expand Down
8 changes: 2 additions & 6 deletions packages/next-swc/turbo.json
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,7 @@
"outputs": ["native/*.node"]
},
"rust-check": {
"dependsOn": [
"rust-check-fmt",
"rust-check-clippy",
"rust-check-napi-rustls"
]
"dependsOn": ["rust-check-fmt", "rust-check-clippy", "rust-check-napi"]
},
"rust-check-fmt": {
"inputs": [
Expand All @@ -116,7 +112,7 @@
"../../rust-toolchain"
]
},
"rust-check-napi-rustls": {
"rust-check-napi": {
"inputs": [
"../../.cargo/**",
"../../crates/**",
Expand Down
19 changes: 12 additions & 7 deletions turbopack/crates/turbo-tasks-fetch/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,21 @@ edition = "2021"
[lib]
bench = false

[features]
default = ["native-tls"]
# Allow to configure specific tls backend for reqwest.
# See top level Cargo.toml for more details.
native-tls = ["reqwest/native-tls"]
rustls-tls = ["reqwest/rustls-tls"]

[lints]
workspace = true

# Enable specific tls features per-target.
#
# Be careful when selecting tls backend, including change default tls backend.
# If you changed, must verify with ALL build targets with next-swc to ensure
# it works. next-swc have various platforms, some doesn't support native (using openssl-sys)
# and some aren't buildable with rustls.
[target.'cfg(all(target_os = "windows", target_arch = "aarch64"))'.dependencies]
reqwest = { workspace = true, features = ["native-tls"] }

[target.'cfg(not(any(all(target_os = "windows", target_arch = "aarch64"), target_arch="wasm32")))'.dependencies]
reqwest = { workspace = true, features = ["rustls-tls"] }

[dependencies]
anyhow = { workspace = true }
reqwest = { workspace = true }
Expand Down
14 changes: 0 additions & 14 deletions turbopack/crates/turbo-tasks-fetch/build.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,5 @@
use turbo_tasks_build::generate_register;

#[cfg(any(feature = "native-tls", feature = "rustls-tls"))]
fn check_tls_config() {
// do nothing
}
#[cfg(not(any(feature = "native-tls", feature = "rustls-tls")))]
fn check_tls_config() {
panic!("You must enable one of the TLS features: native-tls or rustls-tls");
}

fn main() {
generate_register();

// Check if tls feature for reqwest is properly configured.
// Technically reqwest falls back to non-tls http request if none of the tls
// features are enabled, But we won't allow it.
check_tls_config();
}
10 changes: 2 additions & 8 deletions turbopack/crates/turbopack-cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,7 @@ name = "mod"
harness = false

[features]
# By default, we enable native-tls for reqwest via downstream transitive features.
# This is for the convenience of running daily dev workflows, i.e running
# `cargo xxx` without explicitly specifying features, not that we want to
# promote this as default backend. Actual configuration is done when building turbopack-cli.
default = ["custom_allocator", "native-tls"]
default = ["custom_allocator"]
serializable = []
tokio_console = [
"dep:console-subscriber",
Expand All @@ -32,8 +28,6 @@ tokio_console = [
]
profile = []
custom_allocator = ["turbo-tasks-malloc/custom_allocator"]
native-tls = ["turbo-tasks-fetch/native-tls"]
rustls-tls = ["turbo-tasks-fetch/rustls-tls"]

[lints]
workspace = true
Expand All @@ -51,7 +45,7 @@ tokio = { workspace = true, features = ["full"] }
tracing-subscriber = { workspace = true, features = ["env-filter", "json"] }
turbo-tasks = { workspace = true }
turbo-tasks-env = { workspace = true }
turbo-tasks-fetch = { workspace = true, default-features = false }
turbo-tasks-fetch = { workspace = true }
turbo-tasks-fs = { workspace = true }
turbo-tasks-malloc = { workspace = true, default-features = false }
turbo-tasks-memory = { workspace = true }
Expand Down

0 comments on commit 57e732f

Please sign in to comment.