-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Publish individual bins for Linux-arm64 with regression test for cross-build #10606
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
Conversation
|
GNU testsuite comparison: |
This comment was marked as resolved.
This comment was marked as resolved.
|
@oech3 I'll take a look this weekend. I have no time today. |
|
ok. thankyou |
9742559 to
8cfa4ba
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
443d211 to
290ff05
Compare
|
If we publish bins, it should be release profile instead of release-small. It made diff larger... |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
CodSpeed Performance ReportMerging this PR will degrade performance by 3.62%Comparing Summary
Performance Changes
Footnotes
|
Ecordonnier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some comments inline. Please also extend the PR description.
| test -f /tmp/usr/local/bin/uu-tty | ||
| test -f /tmp/usr/local/libexec/uu-coreutils/libstdbuf.* | ||
| sudo apt-get install -y gcc-aarch64-linux-gnu | ||
| RUSTFLAGS="${RUSTFLAGS} -C strip=symbols" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| RUSTFLAGS="${RUSTFLAGS} -C strip=symbols" | |
| export RUSTFLAGS="${RUSTFLAGS} -C strip=symbols" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shellcheck complains if we don't split export. (done at L329)
| test -f /tmp/usr/local/libexec/uu-coreutils/libstdbuf.* | ||
| sudo apt-get install -y gcc-aarch64-linux-gnu | ||
| RUSTFLAGS="${RUSTFLAGS} -C strip=symbols" | ||
| export RUSTFLAGS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| export RUSTFLAGS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref #10606 (comment)
| build_makefile: | ||
| name: Build/Makefile | ||
| needs: [ min_version, deps ] | ||
| permissions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand the "build_makefile" jobs are test jobs. The "build" job at line 627 already has "permissions: contents: write" and already has one line for aarch64-unknown-linux-gnu.
Are you trying to publish files from the build_makefile job instead of from the build job so that you can get libstdbuf.so compiled with feat_external_stdbuf? Or why not use the build job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it not rather be something like this in the publish job around line 534?
matrix:
job:
- { os: ubuntu-latest, target: x86_64-unknown-linux-gnu, features: feat_os_unix }
- { os: ubuntu-latest, target: aarch64-unknown-linux-gnu, features: feat_os_unix_gnueabihf, use-cross: true }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x64 individual bins are published by reusing compute_size:. libstdbuf.so is also one of a reason to publish from here.
Ofcause I'm considering to merge jobs for publishing (and deduplicating release build). But currently, I can't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(We should stop/merge duplicated build for compute_size:)
| ! test -f /tmp/usr/local/share/zsh/site-functions/_install | ||
| ! test -f /tmp/usr/local/share/bash-completion/completions/head.bash | ||
| ! test -f /tmp/usr/local/share/fish/vendor_completions.d/cat.fish | ||
| # We publish them instead of discarding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that in order to change what gets released it would be good to have approval from @sylvestre . Can you please open an issue regarding the addition of individual arm binaries to the releases to discuss this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I just don't want to discard bins with release build (not interested in arm).
Cross-build for FreeBSD might better since Linux-arm coreutils is already published.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, 99,999% of users will get uutils coreutils using their package-manager, and the distros like freebsd will use the source code of uutils coreutils and compile it themselves and not use the binary packages. So I am not sure it is useful to publish individual binaries for arm or freebsd? What is the use case? That is why I suggested to create an issue to discuss it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Package manager does not provide binaries from main branch and most distributor provides single binary.
943d6fc to
9f9c52b
Compare
|
GNU testsuite comparison: |
@Ecordonnier