uhd: fix for python module failing to install #306273
Conversation
NixOSInfra
added
the
12. first-time contribution
|
@ofborg eval |
wegank
added
12.approvals: 1
There was a problem hiding this comment.
Hello @systemofapwne ! Thanks for debugging this issue, and attempting to solve it. Also, welcome to Nixpkgs.
It rewrites upstreams derived installation path for the python module (
/nix/store/-uhd/../-python-3.XY-env/lib/python-3.XY/site-packages->/nix/store/-uhd/lib/python-3.XY/site-packages) to avoid permission problems.
I read upstream's CMake files to get some context for your patch, and I think that now, since we have a better understanding of how they work, we can suggest to them a fix that will be acceptable by them as well.
My main comment is below. In general, I dislike using such patches, as they are easy to become inapplicable when versions are bumped. I also read upstream's source code for their master branch, vs the v4.6.0.0 tag, and I'm afraid your patch will become a maintenance burden on the next uhd release, or the releases afterwards.
Additionally, I think that if this enablePythonApi feature will be properly fixed, we can set it to true by default, so that we will be forced to test that it works on every release of uhd.
| DEPENDS ${PYUHD_FILES}) | ||
|
|
||
| add_custom_target(pyuhd_library ALL DEPENDS ${TIMESTAMP_FILE} pyuhd) | ||
| +set(HAVE_PYTHON_VIRTUALENV FALSE) |
There was a problem hiding this comment.
Perhaps instead of this, and the string(REGEX REPLACE, we can modify the command upstream uses here:
? What do you think? Perhaps upstream will also accept such a change..
There was a problem hiding this comment.
Do you have a suggestion on how to utilize upstreams direct call to setuptools for this package here? I am rather fresh to nix and still learning about the details.
I have absolutely the same view on this as you, since I noticed the same change between master and the uhd-4.6 branch. A change of upstream is most welcome. E.g. by allowing an override for the VENV detection by a build parameter. To the regexp: I think this mostly became necessary due to this packages structure of handling the python env: . For UHD 3.5, this regexp was not necessary (https://github.com/NixOS/nixpkgs/blob/nixos-21.11/pkgs/applications/radio/uhd/3.5.nix).Little sidenote: UHD 3.5 (nixpkgs 21.11) was properly installing the uhd python bindungs due to the VENV check failing upstream and the python-env of this package being differently defined. That together lead to my patch. |
|
OK so I investigated this a bit further, and I found a solution already supported by upstream's cmake option |
|
Closed in favour of this more solid PR: #307435 |
Description of changes
This PR fixes the python module failing to install by patching the upstream build process.
I kept the enablePythonApi flag off however, to respect established uhd package default options.
Detail of this fix:
Things done
Only tested on x86_64-linux
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.