Gnuradio: Enable Darwin Support #27344

lukeadams · 2017-07-12T22:06:58Z

Motivation for this change

This PR enables Darwin build support for Gnuradio.

To accomplish this, it also:

fixes qwt6_qt4 package on Darwin
Upgrades libosmocore [0.9.0->0.9.6], with required changes
minor changes including enableParallelBuilding to others

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
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 nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

Continuation of #26144

Mic92

some notes.

Mic92 · 2017-07-12T22:25:45Z

pkgs/applications/misc/gnuradio-ais/default.nix

@@ -26,11 +26,15 @@ stdenv.mkDerivation rec {
    done
  '';

+  patches = [ ./fix-volk.patch ];


Can you also send this patch upstream?

Upstream hasn't had any activity in 2+yr. Macports seems to have taken the same route and included the patch with the build definition

They might accept straight forward build-fixes though. In the worst case, you still raise awareness and somebody can profit from the patch.

bistromath/gr-ais@8502d02

Should I update the file to point to my PR commit?

A link would be nice.

Unless you have an objection, I'm going to change the rev to point to my PR commit, since it removes the need to include the patch here

Mic92 · 2017-07-12T22:28:28Z

pkgs/applications/misc/libosmocore/default.nix

  buildInputs = [
-    autoreconfHook pcsclite pkgconfig
+    autoreconfHook pkgconfig git 


at least autoreconfHook, pkgconfig can be moved to nativeBuildInputs. Is git really required here?

Nope, git isn't required. I added it while resolving a few compilation errors and never removed it

bjornfor

This commit should be split. It does way more than "enabling darwin support". For instance, I don't like that it removes wrapping of the examples, because that breaks them.

bjornfor · 2017-07-14T15:53:52Z

Oh, I thought I reviewed only one commit, but it appears github meant it was for the whole PR.

For the remaining changes, please rebase/squash so that there is one logical change per commit.

lukeadams · 2017-07-15T00:38:10Z

This commit should be split. It does way more than "enabling darwin support". For instance, I don't like that it removes wrapping of the examples, because that breaks them.

I split the gnuradio commit for more clarity and reverted that specific change.

For the remaining changes, please rebase/squash so that there is one logical change per commit.

I also reordered and split changes to dependencies of gnuradio to make the history more clear, in addition to rebasing on top of master.

Hopefully it helps! I apologize for dumping one large commit; I got too aggressive with squashing on my end

bjornfor · 2017-07-15T09:36:52Z

@lukeadams: Thank you! That was easy to review. You even split the commits more than I imagined :-)

Does each commit represent a strict improvement and no regressions? I ask because it seems that e.g. the "qwt6_qt4: fixupPhase: Repair relative reference in darwin framework ..." comes a bit late in the sequence. Perhaps squash into "qwt6_qt4: Fix darwin build [by adding AGL input]" or something? (What constitutes a logical change is always up for discussion. A "Fix Darwin build" that does a bunch of stuff is also valid.)

Sorry if I misread the PR, but my point is simply that each commit moves the code base forward linearly. In general, if one commit says "this fixes the build" but then a later one is also needed to really fix the build, then that's not good. Commits should not be split so much that it breaks "git bisect" either.

lukeadams · 2017-07-15T20:54:40Z

Glad you liked it!
Regarding the qwt commits:
Darwin building was already enabled but failing, and adding AGL allowed the build to succeed. The fixupPhase basically enabled proper linking to it as a dependency. I can still squash them, but the latter doesn't affect the success/failure of the build.

For the rest of it, I made sure any changes required for darwin were committed before enabling darwin building. For example d3d567e has prerequisites for gr-ais darwin compilation, but darwin is not enabled till a later commit. None of the commits break the build, but some may prevent the build from breaking in preparation for darwin building. For some reason Github is showing the commits in the wrong order, which makes it a little confusing to look at. They show up correctly in git log though. Should I try to get the timestamps in order or is it fine?

Is there any way to "test" if git-bisect is broken by any of my commits? Running git bisect start on a clean copy of my branch shows no errors (although I haven't had the chance to use it so I'm not familiar).

thanks for taking the time to review this, this is my first real attempt to contribute to nixpkgs and I really appreciate it

bjornfor · 2017-07-16T08:59:40Z

@lukeadams: Great. Just wanted to check (commit order).

As for "git bisect" checking. I mostly just think about the commit sequence. To actually verify it you have to do some building. Or a lot, depending on PR rebuild footprint and number of commits.

I run nox-review wip --against @{u} && nix-check-before-push.sh, where nix-check-before-push.sh is

nix-env -f . -qa \* --meta --xml --drv-path --show-trace >/dev/null || { echo FAILED; exit 1; }
nix-build pkgs/top-level/release.nix -A tarball || { echo FAILED; exit 1; }

And that you'd have to run on each commit. You can do git checkout myfeature~N && that_check_command_above and change N=0... until you're back at the commit you branched out from.

I tried to build this PR on top of master and got this failure in the end (NixOS, x86_64-linux):

$ nox-review wip --against @{u}
...
209: Test command: /nix/store/1vcp949ka9qnyp6dfv4s9pgjda57vk4x-bash-4.4-p12/bin/sh "/tmp/nix-build-gnuradio-3.7.11.drv-0/gnuradio-3.7.11/build/gr-wavelet/python/wavelet/qa_classify_test.sh"
209: Test timeout computed to be: 9.99988e+06
Scanning dependencies of target gui
209: ....
209: ----------------------------------------------------------------------
209: Ran 4 tests in 0.003s
209:
209: OK
209/209 Test #209: qa_classify ..........................   Passed    0.71 sec

99% tests passed, 1 tests failed out of 209

Total Test time (real) = 208.71 sec

The following tests FAILED:
        196 - qa_qtgui (Failed)
Errors while running CTest
make: *** [Makefile:152: test] Error 8
builder for ‘/nix/store/4syq5hckgf8vdd9f0np395gqxri5cm6r-gnuradio-3.7.11.drv’ failed with exit code 2
error: build of ‘/nix/store/4syq5hckgf8vdd9f0np395gqxri5cm6r-gnuradio-3.7.11.drv’ failed
The invocation of "nix-build -A gnuradio-gsm -A gnuradio-osmosdr -A gnuradio-nacl -A libosmocore -A inspectrum -A gplates -A qwt6_qt4 -A gnuradio -A gnuradio-rds -A gnuradio-with-packages -A gnuradio-ais -A gqrx /home/bfo/proj/code/forks/nixpkgs-master" failed

Some small nitpick I found after a 2nd look. You don't have to fix them, but it'd be nice.

Commit style for version bumps is "qwt6_qt4: 6.1.2 -> 6.1.3", not "qwt6_qt4: [6.1.2->6.1.3]"
Some trailing whitespace added various places. Spot them with "git log -p".

lukeadams · 2017-07-16T21:33:14Z

Extremely insightful! I'll play around with git bisect today when I get the chance.

I just fixed the qa_qtgui failure. The error was cannot connect to X server, so I disabled that specific test on linux since it will ALWAYS fail in a NixOS build environment. (in ad60e6e)

And I might as well fix the style issues! I'll do it while I'm fixing the above.

nixpkgs.urh seems to be enabled on darwin now, but won't compile due to some QT59 build error. This is because urh sets platforms to platforms.unix and this PR enables darwin support for all of its inputs, so I might make urh explicitly linux-only for now.
Also might want to look at #22688 regarding PYTHONPATH

lukeadams · 2017-07-21T23:21:09Z

@bjornfor I resolved the stylistic issues (whitespace and commit message related). I've also disabled that specific failing test on Linux. I'm rebuilding on Darwin and NixOS to be sure nothing broke after rebasing against master.

MacOS Sierra, Nix multiuser
NixOS

Sorry about the extensive reviewing work - I've learned so much from this!

bjornfor · 2017-07-22T11:48:54Z

pkgs/applications/misc/gnuradio/default.nix

@@ -68,7 +68,7 @@ stdenv.mkDerivation rec {
    for file in "$out"/bin/* "$out"/share/gnuradio/examples/*/*.py; do
        wrapProgram "$file" \
            --prefix PYTHONPATH : $PYTHONPATH:$(toPythonPath "$out") \
-            --set MATPLOTLIBRC "$out/share/gnuradio"
+            --set MATPLOTLIBRC "$out/share/gnuradio" ${stdenv.lib.optionalString stdenv.isDarwin " --set DYLD_FRAMEWORK_PATH /System/Library/Frameworks"}


Wrap this line?

--set MATPLOTLIBRC "$out/share/gnuradio" \ ${stdenv.lib.optionalString stdenv.isDarwin ... }

bjornfor · 2017-07-22T11:54:48Z

pkgs/applications/misc/gnuradio-ais/default.nix

-    rev = "1863d1bf8a7709a8dfedb3ddb8e2b99112e7c872";
-    sha256 = "1vl3kk8xr2mh5lf31zdld7yzmwywqffffah8iblxdzblgsdwxfl6";
+    # Upstream PR: https://github.com/bistromath/gr-ais/commit/8502d0252a2a1a9b8d1a71795eaeb5d820684054
+    "rev" = "8502d0252a2a1a9b8d1a71795eaeb5d820684054";


Don't forget to update "version" above.

1863d1b (Dec 20, 2015) is the newest commit on the repo, which is "unversioned"
8502d02 is just my pr that fixes a linking issue on top of the above.
Although the version didn't change, should I change the version to the correct date? ("2015-12-20", the last commit to master)

Yes, version should be the commit date (for unversioned repositories).

It seems weird that version goes back one year. Can you clarify a bit in the commit message what's going on here? Also, perhaps make a point of switching from building upstream branch to a PR.

Not sure where the 2016-08-26 date even originated from as the commit referenced (bistromath/gr-ais@1863d1b) is on the date I changed it to.
But yeah I'll add both for clarification

bjornfor · 2017-07-22T14:50:23Z

I started a build on NixOS (too), and ended up with this result:

...
99% tests passed, 1 tests failed out of 208

Total Test time (real) = 207.77 sec

The following tests FAILED:
        149 - qa_constellation_receiver (Failed)
Errors while running CTest
make: *** [Makefile:152: test] Error 8
builder for ‘/nix/store/6kkc2n98g942m1zapcl5693sdlgh5y95-gnuradio-3.7.11.drv’ failed with exit code 2
error: build of ‘/nix/store/6kkc2n98g942m1zapcl5693sdlgh5y95-gnuradio-3.7.11.drv’ failed
The invocation of "nix-build -A gnuradio-osmosdr -A gnuradio-with-packages -A inspectrum -A qwt6_qt4 -A gqrx -A gnuradio-rds -A gplates -A gnuradio-gsm -A gnuradio-nacl -A gnuradio -A libosmocore -A gnuradio-ais /home/bfo/proj/code/forks/nixpkgs-master" failed

lukeadams · 2017-07-22T16:50:20Z

Hmm, that one (possibly along with a few others) is intermittent based on the amount of CPU usage since it basically generates large amounts of data and operates on the data stream. Although it didn't fail on my NixOS build, I'm going to go through and disable any tests that may cause non-deterministic behavior.

lukeadams · 2017-07-26T18:45:25Z

@bjornfor
Apparently gnuradio-companion wasn't being enabled in nix-multiuser environments due to the wx/pygtk imports failing. ead343b fixes this

I've added a "grSkipTests" list to allow simpler test skipping in the future, in addition to skipping tests which seem to fail regularly. (4494343)

Darwin succeeded without any issues after multiple runs.

NixOS succeeded as well

add new required talloc, python2 inputs; enableParallelBuilding

This prevents CoreFoundation-related crashes

enables gnuradio-companion compilation in headless/nix-multiuser environment

-std=c++11 causes errors with OBJ-C files on Darwin. Inject dynamic compiler flag into every cmake file to work around this.

this version includes GCC6 improvements

- Issue fixed in my PR (bistromath/gr-ais@8502d02) - The commit date of the original src is "2015-12-20" not "2016-08-26" (version hasn't changed)

lukeadams · 2017-11-23T03:48:02Z

@bjornfor I fixed the stylistic issues you mentioned. Let me know if there's anything else!

bjornfor · 2017-11-23T07:23:26Z

@lukeadams: Thanks!

lukeadams · 2017-11-26T05:15:02Z

@bjornfor Thanks for the guidance and help with this PR. I just got one of my workstations reconfigured and was happy to see that it's all in the binary cache for darwin, and it even works on High Sierra! ;)

Mic92 reviewed Jul 12, 2017

View reviewed changes

LnL7 added the 6.topic: darwin Running or building packages on Darwin label Jul 13, 2017

bjornfor requested changes Jul 14, 2017

View reviewed changes

lukeadams force-pushed the gnuradio-update branch 3 times, most recently from 061daa9 to 8d778b9 Compare July 15, 2017 00:35

lukeadams force-pushed the gnuradio-update branch from 8d778b9 to 3a88160 Compare July 15, 2017 00:42

lukeadams force-pushed the gnuradio-update branch 3 times, most recently from 457ec1c to d44497b Compare July 21, 2017 23:18

bjornfor reviewed Jul 22, 2017

View reviewed changes

lukeadams force-pushed the gnuradio-update branch 8 times, most recently from 4c8de22 to 82688be Compare July 26, 2017 18:30

Luke Adams added 10 commits November 22, 2017 16:40

qwt6_qt4: preFixup: Repair relative reference in darwin framework output

97c5cf0

qwt6_qt4: enableParallelBuilding

de51e4a

libosmocore: 0.9.0 -> 0.9.6

b32e8ce

add new required talloc, python2 inputs; enableParallelBuilding

libosmocore: enable Darwin building

9be55b0

urh: platforms to linux only due to qt5 issue on Darwin

4bc189d

rtl-sdr: enable Darwin building; postInstall: only run on Linux

60d95cc

gnuradio: document purpose of package inputs

ba49dfb

gnuradio: use qwt6_qt4 instead of qwt5

b9c426c

gnuradio: reorder build inputs

861773b

gnuradio: add CoreAudio input for Darwin

e81d84f

lukeadams force-pushed the gnuradio-update branch from 3973172 to 55b888f Compare November 22, 2017 22:58

GrahamcOfBorg added 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Nov 22, 2017

Luke Adams added 11 commits November 22, 2017 21:44

gnuradio: wrapProgram: set DYLD_FRAMEWORK_PATH

3cdf430

This prevents CoreFoundation-related crashes

gnuradio: postInstall: PYTHONPATH / DYLD_FRAMEWORK_PATH notes

482d8de

gnuradio: setupHook: enable composition with nix-shell

17908a7

gnuradio: force wxgui and pygtk check to true

6e9140d

enables gnuradio-companion compilation in headless/nix-multiuser environment

gnuradio: Resolve darwin c++11 issues

5d75dbe

-std=c++11 causes errors with OBJ-C files on Darwin. Inject dynamic compiler flag into every cmake file to work around this.

gnuradio: 3.7.11.0 -> 3.7.11.1

1b1c82f

this version includes GCC6 improvements

gnuradio: enable Darwin building

86d953e

gnuradio-ais: Fix Volk library search issue. Fix incorrect version date.

d8d9697

- Issue fixed in my PR (bistromath/gr-ais@8502d02) - The commit date of the original src is "2015-12-20" not "2016-08-26" (version hasn't changed)

gnuradio-{ais,gsm,nacl,osmosdr,rds}: enable Darwin building

b1c666e

gnuradio-{ais,gsm,nacl,osmosdr,rds}: enableParallelBuilding

03193f5

gnuradio-osmosdr: update homepage

367e5f7

lukeadams force-pushed the gnuradio-update branch from 55b888f to 367e5f7 Compare November 23, 2017 03:47

bjornfor merged commit 1e5bdbb into NixOS:master Nov 23, 2017

lukeadams deleted the gnuradio-update branch May 26, 2021 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gnuradio: Enable Darwin Support #27344

Gnuradio: Enable Darwin Support #27344

lukeadams commented Jul 12, 2017 •

edited

Mic92 left a comment

Mic92 Jul 12, 2017

lukeadams Jul 12, 2017

Mic92 Jul 12, 2017 •

edited

lukeadams Jul 12, 2017

Mic92 Jul 13, 2017

lukeadams Jul 13, 2017

lukeadams Jul 13, 2017

Mic92 Jul 12, 2017

lukeadams Jul 12, 2017

bjornfor left a comment

bjornfor commented Jul 14, 2017

lukeadams commented Jul 15, 2017 •

edited

bjornfor commented Jul 15, 2017

lukeadams commented Jul 15, 2017 •

edited

bjornfor commented Jul 16, 2017

lukeadams commented Jul 16, 2017 •

edited

lukeadams commented Jul 21, 2017 •

edited

bjornfor Jul 22, 2017

bjornfor Jul 22, 2017

lukeadams Jul 22, 2017

bjornfor Jul 22, 2017

bjornfor Nov 22, 2017

lukeadams Nov 22, 2017

bjornfor commented Jul 22, 2017

lukeadams commented Jul 22, 2017

lukeadams commented Jul 26, 2017 •

edited

lukeadams commented Nov 23, 2017

bjornfor commented Nov 23, 2017

lukeadams commented Nov 26, 2017

Gnuradio: Enable Darwin Support #27344

Gnuradio: Enable Darwin Support #27344

Conversation

lukeadams commented Jul 12, 2017 • edited

Motivation for this change

Things done

Mic92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Jul 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjornfor left a comment

Choose a reason for hiding this comment

bjornfor commented Jul 14, 2017

lukeadams commented Jul 15, 2017 • edited

bjornfor commented Jul 15, 2017

lukeadams commented Jul 15, 2017 • edited

bjornfor commented Jul 16, 2017

lukeadams commented Jul 16, 2017 • edited

lukeadams commented Jul 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjornfor commented Jul 22, 2017

lukeadams commented Jul 22, 2017

lukeadams commented Jul 26, 2017 • edited

lukeadams commented Nov 23, 2017

bjornfor commented Nov 23, 2017

lukeadams commented Nov 26, 2017

lukeadams commented Jul 12, 2017 •

edited

Mic92 Jul 12, 2017 •

edited

lukeadams commented Jul 15, 2017 •

edited

lukeadams commented Jul 15, 2017 •

edited

lukeadams commented Jul 16, 2017 •

edited

lukeadams commented Jul 21, 2017 •

edited

lukeadams commented Jul 26, 2017 •

edited