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

gnuradio: 3.7.13.4 -> 3.7.14.0 & more #84401

Closed
wants to merge 50 commits into from

Conversation

doronbehar
Copy link
Contributor

Motivation for this change

Address most of the ideas in #82263 without introducing gnuradio 3.8 yet.

Things done

  • Add gnuradio.plugins. attribute set which has all of the old gr-* gnuradio plugins / blocks.

Gnuradio

cc @bjornfor @fpletz

  • Add many enable / disable flags along with some dependencies cleanup.
  • Use a separate wrapper derivation which uses symlinkJoin.
  • Add 2 more build flavors - 1 with all features enabled and another with most features but no gui

gr-* plugins

cc @mog @markuskowa @bjornfor @the-kenny

  • Use src appropriate for gnuradio version used.
  • Add aliases which throw a message suggesting to use the new attribute gnuradio.plugins.*.

qradiolink

cc @markuskowa

inspectrum

cc @mog

  • Use the new appropriate build flavor of gnuradio as dependency.

gnss-sdr

  • Add myself as maintainer (it lacked a maintainer before this change).
  • Fix python unfound issue with build (no idea how it was able to build before).
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip" nix-build -A.
  • 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.

Last Notes

  • There should some TODO comments with questions, I'll be happy to hear an opinion regarding these small decisions.
  • Although this PR is big, I tried as hard as I could to make it easy as possible to review. Maintainers please use the commit log to see what changes were made in your domain, I prefixed each commit with the relevant package.

@doronbehar
Copy link
Contributor Author

@wirew0rm I think you'd be interested in this PR.

@flokli
Copy link
Contributor

flokli commented Apr 14, 2020

Impressive work! Some questions:

  • Is it feasible to join the common logic from 3.7.nix and 3.8.nix?
  • There's now 7 derivations (+plugins attrset) x2 versions:
  • unwrapped${version}, unwrapped${version}-full, unwrapped${version}-no-gui, gnuradio${version}-no-gui, gnuradio${version}, gnuradio${version}-with-packages, gnuradio${version}-full. This looks very complex, and I don't quite understand when to use what. Do we really need all of these?
  • I assume some of the different variations exist to get closure sizes down. Is it possible to move this into multiple outputs, like moving python, companion, qtgui, wxgui and utils into separate outputs?
  • IMHO, gnuradio should always point to the latest released version - we can add a release note entry and still provide gnuradio_3_7 as a fallback.

@doronbehar
Copy link
Contributor Author

Is it feasible to join the common logic from 3.7.nix and 3.8.nix?

Very likely. Moreover, it should be possible to make all of the components-cmakeFlags-dependencies mechanism work better and in a more declarative manner. Perhaps similarly to mpd.

I wasn't as experienced in Nix expressions as I'm today (after working on #85103) when I wrote this PR, so I'll have to work on this further.

There's now 7 derivations (+plugins attrset) x2 versions

It's not that many :) the unwrapped derivations are used internally and not exported directly. See the list near the inherit in all-packages.nix to see which ones are made available.

I assume some of the different variations exist to get closure sizes down. Is it possible to move this into multiple outputs, like moving python, companion, qtgui, wxgui and utils into separate outputs?

@flokli you've managed to somewhat change my mind after seeing in #84663 that buildInputs are not necessarily referenced in all outputs :) But I'm still not totally sure how I feel about this.

This is somewhat off topic for this PR but I think it's still appropriate: Generally, ever since I started contributing to Nixpkgs, it really frustrated me that you have to compile a whole derivation from scratch just because something is not working as you wish in the fixup phase where the outputs are split. I consider this a developer-experience issue of Nixpkgs it self, though I have absolutely no idea how to tackle it.

So to summarize my answer to your question, I'm not objected to split the outputs, but I'm not up to do it :/.

IMHO, gnuradio should always point to the latest released version - we can add a release note entry and still provide gnuradio_3_7 as a fallback.

I agree, but I assume the release notes should be updated then? I'll take care of that once the whole PR is truly ready and more information will be required there.


Besides that, just to note my self and anyone else, GNU Radio 3.8's wrapper is not ready yet. I've learned a lot about wrappers after working on #85103 so I should be able to fix GNU Radio's wrapping as well at some point.

@flokli
Copy link
Contributor

flokli commented May 23, 2020

@doronbehar I really like some of the improvements in this PR.

What do you think about reducing the scope a little bit? For example, if 3.8 isn't really ready yet, we don't necessarily need to introduce it (and fixing more things is easier, as there's less code).

@doronbehar
Copy link
Contributor Author

doronbehar commented May 23, 2020

@doronbehar I really like some of the improvements in this PR.

What do you think about reducing the scope a little bit? For example, if 3.8 isn't really ready yet, we don't necessarily need to introduce it (and fixing more things is easier, as there's less code).

Thanks for the encouragement @flokli . I agree it's a good idea, I was putting it all together in a single PR because I was in a sprint. If I'll split this, I hope it'll be OK to ping you in each such PR for your review.

As a start, I need your help with #84243 🙏 - it's an optional dependency of GNURadio 3.7. Here is a rough plan of the split PRs I'd like to send next. Feel free to comment this if you think further splitting is needed.

PR 1

After that, I'll write a PR that will do nothing besides formatting the inputs and the arguments of the current gnuradio/default.nix as in 1dba7ee . The idea is, that it should be super easy to review it and merge it quickly by observing that the formatting didn't change anything from Nix' point of view, meaning nix-build -A gnuradio will just pull the derivation from cache.nixos.org as if nothing has changed. After that, the diffs of the future PRs will be easier to review.

Note

You should note, that today, in current master, GNUradio's frontend is essentially broken - see #87510 so don't get the impression that the near PRs will fix it - the problem is in the wrapper and it should be taken care of only when the plugins and many other changes will be sorted out.

PR 2

A dumb version bump: gnuradio: 3.7.13.4 -> 3.7.14.0 as in 92dadf5 + checking with nixpkgs-review that all dependent packages still build with this update.

Further PRs

Let's talk more after these 2 PRs will be completed. I'll close this PR in the meantime, though it's still good as a reference.

@doronbehar doronbehar closed this May 23, 2020
@doronbehar doronbehar mentioned this pull request May 31, 2020
10 tasks
@doronbehar doronbehar deleted the update-gnuradio3_7 branch December 3, 2021 09:35
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.

None yet

2 participants