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

refactor(language.rust): check wasmwasi target and return error if mismatched #1382

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 2 additions & 2 deletions .fastly/config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ toolchain_constraint = ">= 1.21" # Go toolchain constraint for use wit
toolchain_constraint_tinygo = ">= 1.18" # Go toolchain constraint for use with TinyGo.

[language.rust]
toolchain_constraint = ">= 1.56.1, <1.84.0"
wasm_wasi_target = "wasm32-wasi"
toolchain_constraint = ">= 1.78.0"
wasm_wasi_target = "wasm32-wasip1"

[wasm-tools]
ttl = "24h"
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/pr_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ jobs:
key: ${{ runner.os }}-test-go-mod-${{ hashFiles('**/go.sum') }}
- name: "Install Rust"
uses: dtolnay/[email protected]
- name: "Add wasm32-wasi Rust target"
run: rustup target add wasm32-wasi --toolchain 1.83.0
- name: "Add wasm32-wasip1 Rust target"
run: rustup target add wasm32-wasip1 --toolchain 1.83.0
- name: "Validate Rust toolchain"
run: rustup show && rustup target list --installed --toolchain 1.83.0
shell: bash
Expand Down
3 changes: 2 additions & 1 deletion Dockerfile-rust
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ FROM rust:latest
LABEL maintainer="Fastly OSS <[email protected]>"

ENV RUST_TOOLCHAIN=1.83.0
ENV WASM_WASI_RELEASE=wasm32-wasip1
RUN rustup toolchain install ${RUST_TOOLCHAIN} \
&& rustup target add wasm32-wasi --toolchain ${RUST_TOOLCHAIN} \
&& rustup target add ${WASM_WASI_RELEASE} --toolchain ${RUST_TOOLCHAIN} \
&& apt-get update && apt-get install -y curl jq && apt-get -y clean && rm -rf /var/lib/apt/lists/* \
&& export FASTLY_CLI_VERSION=$(curl -s https://api.github.com/repos/fastly/cli/releases/latest | jq -r .tag_name | cut -d 'v' -f 2) \
GOARCH=$(dpkg --print-architecture) \
Expand Down
2 changes: 1 addition & 1 deletion TESTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ TEST_COMPUTE_BUILD_RUST=1 make test TEST_ARGS="-run TestBuildRust/fastly_crate_p

When running the tests locally, if you don't have the relevant language ecosystems set-up properly then the tests will fail to run and you'll need to review the code to see what the remediation steps are, as that output doesn't get shown when running the test suite.

> **NOTE**: you might notice a discrepancy between CI and your local environment which is caused by the difference in Rust toolchain versions as defined in .github/workflows/pr_test.yml which specifies the version required to be tested for in CI. Running `rustup toolchain install <version>` and `rustup target add wasm32-wasi --toolchain <version>` will resolve any failing integration tests you may be running locally.
> **NOTE**: you might notice a discrepancy between CI and your local environment which is caused by the difference in Rust toolchain versions as defined in .github/workflows/pr_test.yml which specifies the version required to be tested for in CI. Running `rustup toolchain install <version>` and `rustup target add wasm32-wasip1 --toolchain <version>` will resolve any failing integration tests you may be running locally.

To the run the full test suite:

Expand Down
42 changes: 35 additions & 7 deletions pkg/commands/compute/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ func TestBuildRust(t *testing.T) {
Profiles: testutil.TokenProfile(),
Language: config.Language{
Rust: config.Rust{
ToolchainConstraint: ">= 1.54.0",
WasmWasiTarget: "wasm32-wasi",
ToolchainConstraint: ">= 1.78.0",
WasmWasiTarget: "wasm32-wasip1",
},
},
},
Expand Down Expand Up @@ -102,8 +102,8 @@ func TestBuildRust(t *testing.T) {
Profiles: testutil.TokenProfile(),
Language: config.Language{
Rust: config.Rust{
ToolchainConstraint: ">= 1.54.0",
WasmWasiTarget: "wasm32-wasi",
ToolchainConstraint: ">= 1.78.0",
WasmWasiTarget: "wasm32-wasip1",
},
},
},
Expand All @@ -123,6 +123,34 @@ func TestBuildRust(t *testing.T) {
build = "echo no compilation happening"`,
wantRemediationError: compute.DefaultBuildErrorRemediation,
},
{
name: "wasmwasi target error",
args: args("compute build --verbose"),
applicationConfig: &config.File{
Profiles: testutil.TokenProfile(),
Language: config.Language{
Rust: config.Rust{
ToolchainConstraint: ">= 1.78.0",
WasmWasiTarget: "wasm32-wasi",
},
},
},
cargoManifest: `
[package]
name = "fastly-compute-project"
version = "0.1.0"

[dependencies]
fastly = "=0.6.0"`,
fastlyManifest: fmt.Sprintf(`
manifest_version = 2
name = "test"
language = "rust"

[scripts]
build = "%s"`, fmt.Sprintf(compute.RustDefaultBuildCommand, compute.RustDefaultPackageName, compute.RustWasmWasiTarget)),
wantError: "your WasmWasi target in your config was wasm32-wasi, please change it to wasm32-wasip1",
},
// NOTE: This test passes --verbose so we can validate specific outputs.
{
name: "successful build",
Expand All @@ -131,8 +159,8 @@ func TestBuildRust(t *testing.T) {
Profiles: testutil.TokenProfile(),
Language: config.Language{
Rust: config.Rust{
ToolchainConstraint: ">= 1.54.0",
WasmWasiTarget: "wasm32-wasi",
ToolchainConstraint: ">= 1.78.0",
WasmWasiTarget: "wasm32-wasip1",
},
},
},
Expand All @@ -149,7 +177,7 @@ func TestBuildRust(t *testing.T) {
language = "rust"

[scripts]
build = "%s"`, fmt.Sprintf(compute.RustDefaultBuildCommand, compute.RustDefaultPackageName)),
build = "%s"`, fmt.Sprintf(compute.RustDefaultBuildCommand, compute.RustDefaultPackageName, compute.RustWasmWasiTarget)),
wantOutput: []string{
"Creating ./bin directory (for Wasm binary)",
"Built package",
Expand Down
18 changes: 13 additions & 5 deletions pkg/commands/compute/language_rust.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ import (
// NOTE: In the 5.x CLI releases we persisted the default to the fastly.toml
// We no longer do that. In 6.x we use the default and just inform the user.
// This makes the experience less confusing as users didn't expect file changes.
const RustDefaultBuildCommand = "cargo build --bin %s --release --target wasm32-wasi --color always"
const RustDefaultBuildCommand = "cargo build --bin %s --release --target %s --color always"

// RustWasmWasiTarget is the expected Rust WasmWasi build target
const RustWasmWasiTarget = "wasm32-wasip1"

// RustManifest is the manifest file for defining project configuration.
const RustManifest = "Cargo.toml"
Expand Down Expand Up @@ -144,12 +147,17 @@ func (r *Rust) Dependencies() map[string]string {

// Build compiles the user's source code into a Wasm binary.
func (r *Rust) Build() error {
var wasmWasiTarget = r.config.WasmWasiTarget
if wasmWasiTarget != RustWasmWasiTarget {
return fmt.Errorf("your WasmWasi target in your config was %s, please change it to %s", wasmWasiTarget, RustWasmWasiTarget)
Copy link
Member

Choose a reason for hiding this comment

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

This helps to get it compiled but maybe it should also list the other required compute project changes. I am not sure about that just something to think about. Also do we already have a public page with instructions or is this something we decided not to do anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question, the vibe i was getting is that we generally don't expect people to provide their own configs and that they'll use the default. But it's good to check that assumption, i'll poke around

}

if r.build == "" {
r.build = fmt.Sprintf(RustDefaultBuildCommand, RustDefaultPackageName)
r.build = fmt.Sprintf(RustDefaultBuildCommand, RustDefaultPackageName, wasmWasiTarget)
r.defaultBuild = true
}

err := r.modifyCargoPackageName(r.defaultBuild)
err := r.modifyCargoPackageName(r.defaultBuild, wasmWasiTarget)
if err != nil {
return err
}
Expand Down Expand Up @@ -204,7 +212,7 @@ type RustToolchain struct {
// modifyCargoPackageName validates whether the --bin flag matches the
// Cargo.toml package name. If it doesn't match, update the default build script
// to match.
func (r *Rust) modifyCargoPackageName(noBuildScript bool) error {
func (r *Rust) modifyCargoPackageName(noBuildScript bool, wasmWasiTarget string) error {
s := "cargo locate-project --quiet"
args := strings.Split(s, " ")

Expand Down Expand Up @@ -294,7 +302,7 @@ func (r *Rust) modifyCargoPackageName(noBuildScript bool) error {

// Ensure the default build script matches the Cargo.toml package name.
if noBuildScript && r.packageName != "" && r.packageName != RustDefaultPackageName {
r.build = fmt.Sprintf(RustDefaultBuildCommand, r.packageName)
r.build = fmt.Sprintf(RustDefaultBuildCommand, r.packageName, wasmWasiTarget)
}

return nil
Expand Down
Loading