Skip to content

app-containers/incus: fix cross compilation issue#36323

Closed
tormath1 wants to merge 1 commit intogentoo:masterfrom
tormath1:tormath1/incus
Closed

app-containers/incus: fix cross compilation issue#36323
tormath1 wants to merge 1 commit intogentoo:masterfrom
tormath1:tormath1/incus

Conversation

@tormath1
Copy link
Copy Markdown
Contributor

@tormath1 tormath1 commented Apr 19, 2024

Hi, in this PR we fix some cross-compilation issue in the Incus ebuild:

  • explicitly call the *src_unpack
  • set install path in function of the architecture

Let me know if I should squash the commits.


When cross-compiling to arm64, the Go environment variables were not properly set from the go-env.eclass because the verify-sig src_unpack method was overriding the src-unpack method from the go-module.eclass.

Reference: flatcar/scripts#1655

@gentoo-bot
Copy link
Copy Markdown

Pull Request assignment

Submitter: @tormath1
Areas affected: ebuilds
Packages affected: app-containers/incus

app-containers/incus: @juippis, @gentoo/virtualization

Linked bugs

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment.

If you do not receive any reply to this pull request, please open or link a bug to attract the attention of maintainers.


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Apr 19, 2024
@gentoo-repo-qa-bot
Copy link
Copy Markdown
Collaborator

Pull request CI report

Report generated at: 2024-04-19 13:44 UTC
Newest commit scanned: 1e39411
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/9ab7f547db/output.html

Copy link
Copy Markdown
Member

@juippis juippis left a comment

Choose a reason for hiding this comment

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

Let's only focus on 6.0.0-r1, older versions should've been dropped already from the tree. I can cherry-pick the 2 commits and squash them.

Can you show me a full build log from a successful build on arm64 system (that I guess is cross-compiled)?

export GOPATH="${S}/_dist"
local bindir="_dist/bin"

if [[ "${GOARCH}" == "arm64" ]]; then
Copy link
Copy Markdown
Member

@Flowdalic Flowdalic Apr 19, 2024

Choose a reason for hiding this comment

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

I wonder if this shoudn't be more generic, and if it would break things if we are non-cross compiling on arm64:

Suggested change
if [[ "${GOARCH}" == "arm64" ]]; then
if is_crosscompile; then

(from toolchain.eclass)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I finally used the opposite (if GOARCH is not amd64) and used tc-is-cross-compile for the bash completion generation.

* explicitly call the *src_unpack
* set Go installation PATH if cross compilation is detected

Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
@tormath1
Copy link
Copy Markdown
Contributor Author

Let's only focus on 6.0.0-r1, older versions should've been dropped already from the tree. I can cherry-pick the 2 commits and squash them.

Can you show me a full build log from a successful build on arm64 system (that I guess is cross-compiled)?

@juippis thanks for the reply. Commits have been squashed and only 6.0.0 has been updated.
Here's an output of a cross-compiled Incus: app-containers:incus-6.0.0:20240422-090603.log

It has been tested on Flatcar CI and it emerges correctly for both arch.

Comment on lines +189 to +195
if ! tc-is-cross-compiler; then
# Generate and install shell completion files.
mkdir -p "${D}"/usr/share/{bash-completion/completions/,fish/vendor_completions.d/,zsh/site-functions/} || die
"${bindir}"/incus completion bash > "${D}"/usr/share/bash-completion/completions/incus || die
"${bindir}"/incus completion fish > "${D}"/usr/share/fish/vendor_completions.d/incus.fish || die
"${bindir}"/incus completion zsh > "${D}"/usr/share/zsh/site-functions/_incus || die
fi
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You modify the bindir already, isn't this useless now? Whether or not crosscompiling it should resolve to by the former function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The issue here is more about using the produced binaries: if you target arm64 and your host is amd64 then calling the incus binary just won't work.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm gonna add an ewarn here about shell completion files not being installed. Rest looks good I guess, thanks!

@gentoo-repo-qa-bot
Copy link
Copy Markdown
Collaborator

Pull request CI report

Report generated at: 2024-04-22 16:34 UTC
Newest commit scanned: 31e1c44
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/b36a1e302d/output.html

Comment on lines +189 to +195
if ! tc-is-cross-compiler; then
# Generate and install shell completion files.
mkdir -p "${D}"/usr/share/{bash-completion/completions/,fish/vendor_completions.d/,zsh/site-functions/} || die
"${bindir}"/incus completion bash > "${D}"/usr/share/bash-completion/completions/incus || die
"${bindir}"/incus completion fish > "${D}"/usr/share/fish/vendor_completions.d/incus.fish || die
"${bindir}"/incus completion zsh > "${D}"/usr/share/zsh/site-functions/_incus || die
fi
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm gonna add an ewarn here about shell completion files not being installed. Rest looks good I guess, thanks!

@tormath1 tormath1 deleted the tormath1/incus branch April 23, 2024 08:10
@juippis
Copy link
Copy Markdown
Member

juippis commented Apr 23, 2024

https://bugs.gentoo.org/930496

I think I'm gonna try with @Flowdalic's suggestion about modifying the bindir only when tc-is-crosscompiler is detected. Please test it so it works once it's committed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants