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

packit.yaml: run tests on a different architecture (aarch64) #536

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

jelly
Copy link
Member

@jelly jelly commented Jun 7, 2024

No description provided.

@jelly jelly requested a review from martinpitt June 7, 2024 12:55
martinpitt
martinpitt previously approved these changes Jun 7, 2024
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Nice! (Obviously tests have the last word)

@martinpitt
Copy link
Member

OK, it reproduces the koji failure, so easier to iterate here.

@jelly
Copy link
Member Author

jelly commented Jun 7, 2024

OK, it reproduces the koji failure, so easier to iterate here.

Yup, time to figure out why it works in podman/cockpit :)

So the difference here is that our %build step actually builds while in podman's case it is empty.

Files

%build
NODE_ENV=production make

%install
%make_install PREFIX=/usr

# drop source maps, they are large and just for debugging
find %{buildroot}%{_datadir}/cockpit/ -name '*.map' | xargs --no-run-if-empty rm --verbose

Podman

%prep
%setup -q -n %{name}

%build
# Nothing to build

%install
%make_install PREFIX=/usr
appstream-util validate-relax --nonet %{buildroot}/%{_datadir}/metainfo/*

@jelly
Copy link
Member Author

jelly commented Jun 7, 2024

So naively I tried to just remove the x64 binary, but that breaks package.json

[root@fedora-aarch64-test cockpit-files]# ./build.js

node:internal/process/esm_loader:34
      internalBinding('errors').triggerUncaughtException(
                                ^
Error [TransformError]: The package "@esbuild/linux-arm64" could not be found, and is needed by esbuild.

@jelly
Copy link
Member Author

jelly commented Jun 7, 2024

So nuking the node_modules and running npm install again works:

[root@fedora-aarch64-test cockpit-files]# rm -rf node_modules
[root@fedora-aarch64-test cockpit-files]# npm install
npm WARN deprecated [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm WARN deprecated [email protected]: Rimraf versions prior to v4 are no longer supported
npm WARN deprecated [email protected]: Please use @jridgewell/sourcemap-codec instead
npm WARN deprecated [email protected]: Glob versions prior to v9 are no longer supported

added 467 packages, and audited 468 packages in 37s

166 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
[root@fedora-aarch64-test cockpit-files]# ./build.js
true

So it seems sadly that node_modules are $arch dependent.

[root@fedora-aarch64-test cockpit-files]# ls "node_modules/@esbuild"
[root@fedora-aarch64-test cockpit-files]# ls "node_modules.orig/@esbuild"
linux-x64

The true which is printed comes from ./build.js and prints if the arch is != x64.

@jelly
Copy link
Member Author

jelly commented Jun 7, 2024

Ah, so I read the hint again:

 Or you could consider using yarn
instead of npm which has built-in support for installing a package on multiple
platforms simultaneously.

Everything is pretty much summed up here https://esbuild.github.io/getting-started/#simultaneous-platforms

@jelly jelly force-pushed the test-aarch64 branch 2 times, most recently from ed7818f to 7c45820 Compare June 7, 2024 15:27
@martinpitt martinpitt marked this pull request as draft June 9, 2024 07:17
@jelly
Copy link
Member Author

jelly commented Jun 19, 2024

This is now more or less resolved as we package the bundled JS. But @martinpitt should we still test on ARM, it doesn't really hurt us and might find interesting issues with upload?

@martinpitt
Copy link
Member

@jelly Yes, agreed -- these sometimes find funny bugs, and just the first commit here should work fine now? (at least wrt. the build system)

@jelly jelly marked this pull request as ready for review June 20, 2024 09:29
@jelly
Copy link
Member Author

jelly commented Jun 20, 2024

@jelly Yes, agreed -- these sometimes find funny bugs, and just the first commit here should work fine now? (at least wrt. the build system)

👍 re-pushed, this should go green 🤞

@jelly jelly requested a review from martinpitt June 20, 2024 13:20
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Wohoo!

@martinpitt martinpitt merged commit 84d8dc5 into cockpit-project:main Jun 20, 2024
16 checks passed
@jelly jelly deleted the test-aarch64 branch August 19, 2024 07:39
@jelly jelly mentioned this pull request Aug 19, 2024
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.

2 participants