Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

Run ci smoke tests on armv7 #59

Merged
merged 1 commit into from
Sep 17, 2021
Merged

Run ci smoke tests on armv7 #59

merged 1 commit into from
Sep 17, 2021

Conversation

bonomat
Copy link
Collaborator

@bonomat bonomat commented Sep 15, 2021

Imho it's a common assumption that our tool will run on a RPi and we should ensure that it keeps building for this platform.

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.

This patch doesn't actually do anything useful because I've optimized things away in b5fe043 😅

We will need to add usage of ${{ matrix.target }} again!

@bonomat bonomat marked this pull request as draft September 16, 2021 06:18
@bonomat
Copy link
Collaborator Author

bonomat commented Sep 16, 2021

Converted to draft because something is not working:

  • the project builds on arm
  • the tests run on arm
  • executing the binaries fails (see last commit)

Any idea?

@thomaseizinger
Copy link
Contributor

Any idea?

You can't run an ARM binary on a x64 processor. You would need to start up an emulator like QEMU to do that. In the past we simply skipped the smoke test for certain targets.

@bonomat
Copy link
Collaborator Author

bonomat commented Sep 16, 2021

Any idea?

You can't run an ARM binary on a x64 processor. You would need to start up an emulator like QEMU to do that. In the past we simply skipped the smoke test for certain targets.

Oh makes sense, I didn't know that all targets run on x86_64 architectures.

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.

Thanks!

A question + suggestion!

@@ -0,0 +1,4 @@
[toolchain]
channel = "1.54"
Copy link
Contributor

Choose a reason for hiding this comment

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

1.55 is the latest release, any reason why you want 1.54 here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No particular reason. Changed to 1.55 and updated locally :)

@@ -0,0 +1,4 @@
[toolchain]
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have a rust-toolchain.toml file, it would be good to do an explicit install step instead of "implying" this with the first call to cargo. Reason being that the cache action works out the cache key based on the installed rust version and that would happen before the first cargo invocation, effectively creating the wrong cache key for what is actually being used.

See actions-rs/toolchain#126 + recommended solution.

We cannot run the smoke tests for arm as all targets on Github are x86_64
@bonomat bonomat merged commit b0e31f0 into get10101:master Sep 17, 2021
@bonomat bonomat deleted the armv7 branch September 17, 2021 07:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants