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: use host architecture to determine FreeBSD ABI #350

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

defanator
Copy link
Contributor

Proposed changes

This change allows to get working FreeBSD agent packages on both x86 and arm64 hosts/runners when running make local-txz-package target.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING document
  • If applicable, I have tested my cross-platform changes (verified manually on FreeBSD 13.2 amd64/aarch64)

@netlify
Copy link

netlify bot commented Jun 20, 2023

Deploy Preview for agent-public-docs ready!

Name Link
🔨 Latest commit 5e8c091
🔍 Latest deploy log https://app.netlify.com/sites/agent-public-docs/deploys/64a2bef6b46aaa00080eee04
😎 Deploy Preview https://deploy-preview-350--agent-public-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the bug Something isn't working label Jun 20, 2023
@oliveromahony
Copy link
Contributor

We don't support arm in FreeBSD in NGINX Plus see: https://docs.nginx.com/nginx/technical-specs/

Is this PR needed?

@defanator
Copy link
Contributor Author

@oliveromahony uhm not sure what's the relationship with N+ here; I was working on the fix in #352 and realized that NFPM produces broken .pkg/.txz on M1 macbook. This PR fixes this in a way when you'll get same arch package on any host.

@oliveromahony
Copy link
Contributor

We generally deploy on the same host/container as NGINX. We use Plus as the reference in our support matrices we test against. So for us, supporting arm64 isn't required for the packages we build. The local changes I'm ok with if you want to test on Mac M1/2 but we won't produce official arm64 packages for FreeBSD for the moment unless Plus supports that architecture

@@ -4,6 +4,13 @@ set -e
set -x
set -euxo pipefail

case "$(uname -m)" in
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have OSARCH variable in the root Makefile you can leverage. It should take from that value instead of adding more logic here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm I don't see any relation between Makefile arg(s), which are not even being exported, and entrypoint scripts. Moreover, with multiarch containers (e.g. buildx) you'll get broken entrypoint for archs not matching host's arch if you will be relying on OSARCH obtained on host. Finally, OSARCH is being weirdly transformed from uname -m (introduced here - 24a158b#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L36), so I wouldn't rely on that at all for the sake of simplicity.

@defanator
Copy link
Contributor Author

but we won't produce official arm64 packages for FreeBSD for the moment unless Plus supports that architecture

@oliveromahony roger that - I'm not asking you to do so; but if/when N+ will ship aarch64 binaries for FreeBSD, agent changes will be minimal with this PR (basically, you'll just need to use full path with ABI/arch component for resulting .pkgs, that's all).

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@95054a2). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #350   +/-   ##
=======================================
  Coverage        ?   67.25%           
=======================================
  Files           ?      113           
  Lines           ?    12714           
  Branches        ?        0           
=======================================
  Hits            ?     8551           
  Misses          ?     3590           
  Partials        ?      573           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dhurley dhurley merged commit 996d150 into nginx:main Jul 27, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants