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(interop-tests): don't hardcode x86_64 for native #4862

Merged
merged 4 commits into from
Nov 16, 2023

Conversation

diegomrsantos
Copy link
Contributor

@diegomrsantos diegomrsantos commented Nov 14, 2023

Description

Remove hard-coded x86_64 for the native interop tests. The tests couldn't run on arm64 architecture.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@diegomrsantos diegomrsantos changed the title fix(interop-tests): remove hard-coded x86_64 for rust v0.52 interopt fix(interop-tests): remove hard-coded x86_64 for rust v0.52 interop tests Nov 14, 2023
@diegomrsantos diegomrsantos changed the title fix(interop-tests): remove hard-coded x86_64 for rust v0.52 interop tests fix(interop-tests): remove hard-coded x86_64 for rust v0.52 Nov 14, 2023
@diegomrsantos
Copy link
Contributor Author

@thomaseizinger https://github.com/libp2p/rust-libp2p/actions/runs/6865627713/job/18670058439?pr=4862 is failing, but I believe this is unrelated to my change, isn't it?

@thomaseizinger
Copy link
Contributor

@thomaseizinger libp2p/rust-libp2p/actions/runs/6865627713/job/18670058439?pr=4862 is failing, but I believe this is unrelated to my change, isn't it?

It is related to your change because I'd assume that the base image no longer pulls in the correct Rust version.

@thomaseizinger
Copy link
Contributor

I actually think we should target master here. I've been meaning to replace the version that test-plans references to actually use the v0.53 release but I didn't get to it yet.

We can fix this up, make a point-release and swap out the version!

@diegomrsantos
Copy link
Contributor Author

diegomrsantos commented Nov 14, 2023

It is related to your change because I'd assume that the base image no longer pulls in the correct Rust version.

But I changed the native docker file, and I believe it is failing for the chromium one.

@thomaseizinger
Copy link
Contributor

It is related to your change because I'd assume that the base image no longer pulls in the correct Rust version.

But I changed the native docker file, and I believe it is failing for the chromium one.

Oh, I know what the error is. We might have made a mistake and published an MSRV bump as a patch-release of libp2p-identity. In any case, if we target master instead of the backport branch, this problem will go away.

@thomaseizinger thomaseizinger changed the base branch from v0.52 to master November 14, 2023 23:56
@thomaseizinger thomaseizinger changed the title fix(interop-tests): remove hard-coded x86_64 for rust v0.52 fix(interop-tests): don't hardcode x86_64 Nov 14, 2023
@thomaseizinger thomaseizinger changed the title fix(interop-tests): don't hardcode x86_64 fix(interop-tests): don't hardcode x86_64 Nov 14, 2023
@thomaseizinger
Copy link
Contributor

I rebased your branch onto master but it doesn't work yet as we are currently still hardcoding x64 there. I'll look into what we can do against that ..

@diegomrsantos
Copy link
Contributor Author

diegomrsantos commented Nov 15, 2023

Could you please elaborate more on what your plan is? I believe I can either remove or fix the Rust 0.51 and 0.52 versions used on test-plans, otherwise, I can't run the tests on my machine.

@thomaseizinger
Copy link
Contributor

Could you please elaborate more on what your plan is? I believe I can either remove or fix the Rust 0.51 and 0.52 versions used on test-plans, otherwise, I can't run the tests on my machine.

We've just recently cut the v0.53 release. My plan is to fix ARM compatibility for that one and support running on ARM going forward from that release.

@thomaseizinger thomaseizinger marked this pull request as ready for review November 15, 2023 22:01
@thomaseizinger thomaseizinger changed the title fix(interop-tests): don't hardcode x86_64 fix(interop-tests): don't hardcode x86_64 for native Nov 15, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

@diegomrsantos Can you test that this dockerfile works for you on ARM? Happy to merge in that case.

@diegomrsantos
Copy link
Contributor Author

diegomrsantos commented Nov 15, 2023

Do you plan to remove 0.51 and 0.52 from the transport tests in test-plans?

@diegomrsantos
Copy link
Contributor Author

diegomrsantos commented Nov 15, 2023

I confirm this Dockerfile works on my arm. But I had to change the commitSha in the Makefile inside the rust 0.52 folder to point to the last commit here and comment the 0.51 version in versions.ts in test-plans.

@thomaseizinger
Copy link
Contributor

Do you plan to remove 0.51 and 0.52 from the transport tests in test-plans?

Not yet but eventually, they'll be removed.

@thomaseizinger
Copy link
Contributor

I confirm this Dockerfile works on my arm. But I had to change the commitSha in the Makefile inside the rust 0.52 folder to point to the last commit here and comment the 0.51 version in versions.ts in test-plans.

Do you know that you can provide a name-filter to the test runner to not run all tests?

@diegomrsantos
Copy link
Contributor Author

I didn't, I think it isn't described in the README, could you please share? Could you also please elaborate on why you don't want to fix the current 0.51 and 0.52 versions if they won't be removed soon? Not sure how often it happens, but if other people try to run the tests on arm they will face the same issues.

@thomaseizinger
Copy link
Contributor

I didn't, I think it isn't described in the README, could you please share?

Here is the argument parsing: https://github.com/libp2p/test-plans/blob/a87f0ad99880c933b1f7b54282588a2aa56a4501/transport-interop/testplans.ts#L13-L37

Could you also please elaborate on why you don't want to fix the current 0.51 and 0.52 versions if they won't be removed soon? Not sure how often it happens, but if other people try to run the tests on arm they will face the same issues.

I am just not sure it is worth the effort. The tests are primarily intended to be run in CI and they already work there today. I am okay with adding it on as a requirement that they have to work on ARM going forward. Once GitHub has ARM-hosted runners, we can even test that in CI.

@mergify mergify bot merged commit caf9da4 into libp2p:master Nov 16, 2023
71 checks passed
@thomaseizinger
Copy link
Contributor

Do you also care about the Chromium-based WASM tests? I believe those will also not run on ARM at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants