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

Fix issue running k8s and cross locally #362

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Oct 20, 2023

This PR addresses two issues after #359 when running make test/k8s-wasmtime locally.

First, the TARGET is not being set properly. Since TARGET was already set when re-invoking $(MAKE) it gets the prior value. Example output:

 -f /home/jstur/projects/runwasi/dist/bin/containerd-shim-wasmtime-v1 ] || make install-wasmtime CARGO=cross PREFIX="/home/jstur/projects/runwasi/dist" OPT_PROFILE="debug"
make[1]: Entering directory '/home/jstur/projects/runwasi'
cross build --target=x86_64-unknown-linux-gnu -p containerd-shim-wasmtime

Second, cross has issues when switching targets: https://github.com/cross-rs/cross/wiki/FAQ#glibc-version-error. I ran into this only my local set up, where the first step of cross (after fixing the above) would error out with

=> => naming to localhost/cross-rs/cross-custom-runwasi:x86_64-unknown-linux-musl-d9964                                                              0.0s
   Compiling libc v0.2.149
   Compiling serde v1.0.189
   Compiling thiserror v1.0.49
error: failed to run custom build command for `libc v0.2.149`

Caused by:
  process didn't exit successfully: `/target/debug/build/libc-01039b2d07357d71/build-script-build` (exit status: 1)
  --- stderr
  /target/debug/build/libc-01039b2d07357d71/build-script-build: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.33' not found (required by /target/debug/build/libc-01039b2d07357d71/build-script-build)

This also adds a clean target which allows for testing this scenario, otherwise several of the steps get skipped since they already compiled.

Copy link
Collaborator

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

I don't think we need a wrapper script.

We are already specifying the target in the Makefile with TARGET_FLAG.
It's just a matter of adding --target-dir=./target/build/$(TARGET) to TARGET_FLAG.

You also need to use the same target-dir in the install-% rule (IIUC, that's why CI failed).

I propose we use a new makefile variable TARGET_DIR, and we set it to ./target/build/$(TARGET) when using cross or to ./target otherwise.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@jsturtevant
Copy link
Contributor Author

looks like latest failures is due to cross setting. This is working locally when I run it, with same setting 👀

/usr/bin/docker run --userns host -e 'PKG_CONFIG_ALLOW_CROSS=1' -e 'XARGO_HOME=/home/runner/.xargo'
 -e 'CARGO_HOME=/home/runner/.cargo' -e 'CROSS_RUST_SYSROOT=/home/runner/.rustup/toolchains/1.72.0-x86_64-unknown-linux-gnu' 
-e 'CARGO_TARGET_DIR=/target'

@jsturtevant jsturtevant force-pushed the fix-local-cluster branch 4 times, most recently from 125da87 to e2cd07a Compare October 20, 2023 18:26
Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

lgtm! I didn't run the commands locally though. Will try it out next week.

Copy link
Collaborator

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

LGTM

Makefile Outdated Show resolved Hide resolved
Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>

Co-authored-by: Jorge Prendes <[email protected]>
Copy link
Collaborator

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

LGTM

@Mossaka
Copy link
Member

Mossaka commented Oct 23, 2023

I am trying to get make test/k8s-wasmtime runnning locally...

@Mossaka Mossaka merged commit 776e81c into containerd:main Oct 25, 2023
39 checks passed
@Mossaka
Copy link
Member

Mossaka commented Oct 25, 2023

Merging it in now. The progress on my side is slow and I don't want to block the PR

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.

3 participants