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.

Sign up for GitHub

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

Jump to bottom

rspamd: fix builds on non-x86_64-linux platforms #103973

Merged
merged 1 commit into from
Nov 16, 2020
Merged

rspamd: fix builds on non-x86_64-linux platforms #103973

merged 1 commit into from
Nov 16, 2020
+2 −1

Conversation

vikanezrimaya
Copy link
Member

@vikanezrimaya vikanezrimaya commented Nov 16, 2020 •
edited
Loading

Motivation for this change

Fixes #102751 - architectures other than x86_64 need some love too! (also, it's a package I use on my production machine and I finally figured out how to fix it)

LuaJIT is built in rspamd only on x86_64-linux, and LuaJIT support became enabled by default in 2.6, breaking builds without it. This commit explicitly disables LuaJIT support on non-x86_64 architectures.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS (i686-linux, x86_64-linux, aarch64-linux)
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
    • nixpkgs#legacyPackages.x86_64-linux.rspamd.tests.rspamd.simple
    • nixpkgs#legacyPackages.i686-linux.rspamd.tests.rspamd.simple
    • nixpkgs#legacyPackages.aarch64-linux.rspamd.tests.rspamd.simple
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @avnik @fpletz @globin

Sorry, something went wrong.

@vikanezrimaya
rspamd: fix builds on non-x86_64-linux platforms
71998dd 
LuaJIT is built in rspamd only on x86_64-linux, and LuaJIT support
became enabled by default in 2.6, breaking builds without it. This
commit explicitly disables LuaJIT support on non-x86_64 architectures.
@vikanezrimaya
Copy link
Member Author

/marvin opt-in
/status needs_reviewer

@marvin-mk2
Copy link

marvin-mk2 bot commented Nov 16, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@marvin-mk2 marvin-mk2 bot added marvin This PR was reviewed by Marvin, a discontinued bot: https://github.com/timokau/marvin-mk2 needs_reviewer (old Marvin label, do not use) labels Nov 16, 2020
@vikanezrimaya vikanezrimaya requested a review from fpletz November 16, 2020 15:19
@marvin-mk2 marvin-mk2 bot added awaiting_reviewer (old Marvin label, do not use) and removed needs_reviewer (old Marvin label, do not use) labels Nov 16, 2020
@vikanezrimaya
Copy link
Member Author

/status needs_reviewer

@marvin-mk2 marvin-mk2 bot added needs_reviewer (old Marvin label, do not use) and removed awaiting_reviewer (old Marvin label, do not use) labels Nov 16, 2020
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 16, 2020
@marvin-mk2 marvin-mk2 bot added awaiting_reviewer (old Marvin label, do not use) and removed needs_reviewer (old Marvin label, do not use) labels Nov 16, 2020
@vikanezrimaya
Copy link
Member Author

@ajs124 this PR uses an experimental bot to accelerate code review. If you would merge this PR if you had permissions, write /status needs_merger in a comment to accelerate the process per marvin-bot's usage manual: https://github.com/timokau/marvin-mk2/blob/deployed/USAGE.md ✨

@marvin-mk2
Copy link

marvin-mk2 bot commented Nov 16, 2020

The PR author cannot set the status to needs_merger. Please wait for an external review.

If you are not the PR author and you are reading this, please review the usage of this bot. You may be able to help. Please make an honest attempt to resolve all outstanding issues before setting to needs_merger.

nh2
nh2 approved these changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@nh2 nh2 left a comment

Choose a reason for hiding this comment

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

Change looks good, but the ofborg is still building, and might not finish because it seems to be building the world on aarch64:

https://logs.nix.ci/?attempt_id=f70c6e0c-25ad-4986-b237-685aa8e6ac5f&key=nixos%2Fnixpkgs.103973

@kisik21 What do I need to tell the Marvin if I do have merge permissions and would merge, but first want to give the ofborg the chance to finish the build (e.g. "merge if CI goes green")?

@vikanezrimaya
Copy link
Member Author

@timokau consider giving Marvin merge perms and an auto-merge command on ofborg approval?

@vikanezrimaya
Copy link
Member Author

Also you could try to build this PR yourself - it's a fairly small change, and if you're on unstable it should build pretty quick :3

@timokau
Copy link
Member

timokau commented Nov 16, 2020

@kisik21 no, I consider that out of scope. Its not necessary for marvin's core functionality ("Making sure your PR gets a review and your reviews don't get lost.") and I don't want to have that kind of responsibility.

A merge bot has been / is discussed a lot though, for example this might interest you.

@nh2 it doesn't have any support for that right now. I could imagine a simply reminder functionality at some point in the future. I have thought about some ofBorg integration anyway (it could ask the PR author to fix ofBorg errors before requesting a reviewer). Reminders are pretty far down the backlog for now though.

@timokau
Copy link
Member

timokau commented Nov 16, 2020

Since it was broken before and I assume @kisik21 has tested this locally, I'll ignore the running ofBorg check now. It has been queued for 6h already.

Thanks for fixing this!

@timokau timokau merged commit 655d913 into NixOS:master Nov 16, 2020
@nh2
Copy link
Contributor

nh2 commented Nov 16, 2020

it doesn't have any support for that right now

@timokau @kisik21 My request isn't to allow the bot to auto-merge (that'd be nice for another day), but to address the fact that the usage doesn't describe what to do. It says:

if the PR has been reviewed and the reviewer would merge this PR if they could, but does not have the permission to do so

so the first thing any commiter is thinking is "'does not have the permission to do so' doesn't apply to me, so what do I do now, well the document doesn't say, so I guess ... nothing?"

If "nothing" is the right answer, it'd still be nice to write that into the doc.

@vikanezrimaya vikanezrimaya deleted the fix-rspamd-non-x86_64-linux branch November 17, 2020 07:31
@timokau
Copy link
Member

timokau commented Nov 17, 2020

Marvin just augments the usual PR review process. So in cases where it can't help (yet), the usual process (or lack thereoff) applies. In my case that would be "keep a tab open for a while, then check if something might be wrong with ofBorg if the check still hasn't passed".

Still, any contribution to or clarification of the contributing document is appreciated, feel free to open a PR :)

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

@nh2 nh2

@ajs124 ajs124

@timokau timokau

@fpletz fpletz

@globin globin

@avnik avnik

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux awaiting_reviewer (old Marvin label, do not use) marvin This PR was reviewed by Marvin, a discontinued bot: https://github.com/timokau/marvin-mk2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rspamd for aarch64 is broken on latest unstable
4 participants
@vikanezrimaya @timokau @nh2 @ajs124