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

volk: fix build for apple silicon #160152

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

hexagonal-sun
Copy link
Contributor

Motivation for this change

This fixes the build of volk for apple M1 machines. Until cpu_features gains darwin support, there is no prospect of running gnuradio with Nix on apple silicon. Credit to @michaelld who's work this is based on.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@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 Feb 15, 2022
@michaelld
Copy link

michaelld commented Feb 15, 2022

FYI Volk 2.5.1 was recently released that allows for either an external cpu_features or the internal version, updated so-as to not require this patch. I don't know what version of cpu_features is provided in NixOS, but if it's recent enough to support M1 / ARM64 then update Volk to the current release & use the external cpu_features. I have not updated Volk in MacPorts yet due to my heavy workload. I hope to get it in the next week or so.

@hexagonal-sun
Copy link
Contributor Author

FYI Volk 2.5.1 was recently released that allows for either an external cpu_features or the internal version, updated so-as to not require this patch.

Ah nice! I think we have a PR for updating volk somewhere. I know that cpu_features upstream has been working to add M1 support, have they got enough to now build volk?

@ofborg ofborg bot requested a review from doronbehar February 16, 2022 19:28
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 and removed 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 Feb 16, 2022
@doronbehar
Copy link
Contributor

FYI Volk 2.5.1 was recently released that allows for either an external cpu_features or the internal version, updated so-as to not require this patch. I don't know what version of cpu_features is provided in NixOS, but if it's recent enough to support M1 / ARM64 then update Volk to the current release & use the external cpu_features. I have not updated Volk in MacPorts yet due to my heavy workload. I hope to get it in the next week or so.

@hexagonal-sun or @michaelld , could either of you check whether the volk from #160121 passes a build at your M1 computers? I noticed that even with volk 2.5.1 the build fails according to ofborg. If I understand correctly, you are saying that I shouldn't apply the patch? Or should we package cpu-features and only then it will be fixed? You are welcome to send a PR to my doronbehar:pkg/gnuradio/update branch.

@SuperSandro2000 SuperSandro2000 merged commit 4c6fb45 into NixOS:master Feb 16, 2022
@doronbehar
Copy link
Contributor

@SuperSandro2000 it'd have been nice if you had read the comment I left, indicating that perhaps this PR could have been superseded by #160121. Plus, the ofborg build for volk on aarch64-darwin failed.

@hexagonal-sun
Copy link
Contributor Author

hexagonal-sun commented Feb 17, 2022

@doronbehar Ah, sorry this got merged. Feel free to revert. Yes, I think we may as well target this at 2.5.1 as we'll have more options. It looks like cpu_features isn't packaged in nixpkgs, I think it makes sense to package that up and make volk use that. What are your thoughts?

@SuperSandro2000
Copy link
Member

@SuperSandro2000 it'd have been nice if you had read the comment I left, indicating that perhaps this PR could have been superseded by #160121. Plus, the ofborg build for volk on aarch64-darwin failed.

I thought this was somehow about gnuradio, unrelated to this one.

@hexagonal-sun
Copy link
Contributor Author

@doronbehar @michaelld I've just checked #160121 and it doesn't build on aarch64-darwin I'm afraid. I've also replaced the cpu_features submodule that volk uses with the very latests commits on the main branch. That doesn't seem to work either. It looks like cpu_features needs more fixes before this works.

@michaelld
Copy link

@hexagonal-sun well drat ... I haven't had time to look at cpu_features since they closed the "support for M1" PR ... guess I need to get there sooner rather than later, as I need to update the Volk port in MacPorts & this will require either an external cpu_features or patching the submodule version

@doronbehar
Copy link
Contributor

I thought this was somehow about gnuradio, unrelated to this one.

It's OK, perhaps I should have been more clear in the PR title. Also it seems that the volk update should be halted for now, according to reports by @hexagonal-sun and @michaelld , thanks for your help.

@doronbehar
Copy link
Contributor

FYI, I updated to volk 3.0.0 for non aarch64-darwin platforms only at: #219179

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

Successfully merging this pull request may close these issues.

5 participants