Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion unified/swift-syntax-rs/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,21 @@ exports_files([".swift-version"])
# Targets in this package require a Swift toolchain (Linux or macOS).
# `select()` gives us OR-of-OSes; other platforms get marked incompatible
# so `bazel build/test //...` skips them cleanly.
#
# The Linux branch additionally requires x86_64: the only registered standalone
# Linux Swift toolchain (`swift_toolchain_exec_ubuntu24.04`, see //:MODULE.bazel)
# is x86_64-only. Without the CPU constraint, Linux/aarch64 targets would stay
# "compatible" and then fail toolchain resolution instead of being skipped.
Comment on lines +10 to +13

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this change needed? And why is this the right approach to this change (instead of registering an Arm64 tool chain)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I have no idea why Copilot chose that solution. A better solution is probably to just register an Arm64 tool chain.

This goes back to your other comment as well -- we probably ought to have someone more intimate with the Bazel setup take a look at this. (Or at the very least once we get close to being production-ready.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Honestly, I have no idea why Copilot chose that solution. A better solution is probably to just register an Arm64 tool chain.

I can have a look at that.

This goes back to your other comment as well -- we probably ought to have someone more intimate with the Bazel setup take a look at this. (Or at the very least once we get close to being production-ready.)

This we should probably do as soon as possible, because I am concerned that the changes here might affect the rest of the build in ways that I do not understand.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
# The Linux branch additionally requires x86_64: the only registered standalone
# Linux Swift toolchain (`swift_toolchain_exec_ubuntu24.04`, see //:MODULE.bazel)
# is x86_64-only. Without the CPU constraint, Linux/aarch64 targets would stay
# "compatible" and then fail toolchain resolution instead of being skipped.

config_setting(
name = "linux_x86_64",
constraint_values = [
"@platforms//os:linux",
"@platforms//cpu:x86_64",
],
)

_SWIFT_SUPPORTED_PLATFORMS = select({
"@platforms//os:linux": [],
":linux_x86_64": [],
"@platforms//os:macos": [],
"//conditions:default": ["@platforms//:incompatible"],
})
Comment on lines +14 to +26

@jketema jketema Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The following is slightly sloppy (as we cannot build on Arm64 Linux), but this is consistent with what we do for code build in other languages. I also don't think this needs a clarifying comment for that reason.

Suggested change
config_setting(
name = "linux_x86_64",
constraint_values = [
"@platforms//os:linux",
"@platforms//cpu:x86_64",
],
)
_SWIFT_SUPPORTED_PLATFORMS = select({
":linux_x86_64": [],
"@platforms//os:macos": [],
"//conditions:default": ["@platforms//:incompatible"],
})
_SWIFT_SUPPORTED_PLATFORMS = select({
"@platforms//os:linux": [],
"@platforms//os:macos": [],
"//conditions:default": ["@platforms//:incompatible"],
})

Expand Down
13 changes: 9 additions & 4 deletions unified/swift-syntax-rs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,15 @@ Requirements:
[`xcode_transition.bzl`](xcode_transition.bzl)), so other targets on macOS
keep using Bazel's default CC toolchain.

The Swift compiler version is read from [`.swift-version`](.swift-version) by
both the Bazel toolchain (`swift.toolchain(swift_version_file = …)`) and the
local build, and is kept in sync with the `swift-syntax` release pinned in
`swift/Package.swift`, so local and CI builds behave identically.
The Swift compiler version in [`.swift-version`](.swift-version) is kept in sync
with the `swift-syntax` release pinned in `swift/Package.swift`. It is honored on
**Linux**, where the hermetic swift.org Bazel toolchain
(`swift.toolchain(swift_version_file = …)`) and the local `cargo`/`swift build`
both read it. On **macOS** it is *not* honored by the Bazel build: `rules_swift`
auto-registers the host `xcode_swift_toolchain`, which uses whichever Swift ships
with the installed Xcode and ignores `.swift-version`. So the pinned version
governs Linux (and local) builds, while the macOS compiler version depends on the
host Xcode.

## Usage

Expand Down
24 changes: 14 additions & 10 deletions unified/swift-syntax-rs/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,20 @@ fn main() {
let manifest_dir = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap());
let swift_dir = manifest_dir.join("swift");

println!(
"cargo:rerun-if-changed={}",
swift_dir.join("Package.swift").display()
);
println!(
"cargo:rerun-if-changed={}",
swift_dir
.join("Sources/SwiftSyntaxFFI/SwiftSyntaxFFI.swift")
.display()
);
// Emitting any `rerun-if-changed` disables Cargo's default whole-package
// scan, so we must list every input to the `swift build` below. Watch the
// package manifests, the pinned dependency lockfile, the pinned compiler
// version, and the entire Swift sources tree (a directory is scanned
// recursively). `.build/` is intentionally *not* watched (it is this
// build's output; watching it would cause perpetual rebuilds).
for input in [
swift_dir.join("Package.swift"),
swift_dir.join("Package.resolved"),
swift_dir.join("Sources"),
manifest_dir.join(".swift-version"),
] {
println!("cargo:rerun-if-changed={}", input.display());
}
println!("cargo:rerun-if-env-changed=SWIFT");
println!("cargo:rerun-if-env-changed=SWIFTC");

Expand Down
9 changes: 7 additions & 2 deletions unified/swift-syntax-rs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,20 @@ unsafe extern "C" {
pub enum ParseError {
/// The provided source contained an interior NUL byte.
NulByte,
/// The Swift side failed to produce a result.
/// The Swift shim returned no result. `SwiftParser` recovers from invalid
/// syntax (it always produces a tree, possibly with error nodes), so this
/// does *not* indicate a syntax error in the source — it means the shim
/// failed to serialize the tree to JSON or to allocate the result string.
SwiftFailure,
}

impl std::fmt::Display for ParseError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ParseError::NulByte => write!(f, "source contained an interior NUL byte"),
ParseError::SwiftFailure => write!(f, "swift-syntax failed to parse the source"),
ParseError::SwiftFailure => {
write!(f, "the swift-syntax shim failed to produce a JSON result")
}
}
}
}
Expand Down