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

cmake: reliably determine python prefix #5148

Merged
merged 2 commits into from
Oct 13, 2021

Conversation

schneider42
Copy link
Member

Based on #5131

This shows where I would like to go with the changes proposed in #5121. It allows to get rid of having to manipulate $PYTHONPATH on Debian/Ubuntu machines after installing GNURadio from source (see 3e7435a)

Copy link
Member

@drmpeg drmpeg left a comment

Choose a reason for hiding this comment

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

Works good. Tested with a custom prefix /opt/gnuradio-3.10.0git/. Python install directory is now /opt/gnuradio-3.10.0git/lib/python3.9/site-packages, which matches UHD.

- distro: 'CentOS 8.3'
containerid: 'gnuradio/ci:centos-8.3-3.9'
cxxflags: ''
ctest_args: '-E "qa_agc|qa_cpp_py_binding|qa_cpp_py_binding_set|qa_ctrlport_probes|qa_qtgui"'
ldpath: /usr/local/lib64/
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it become necessary to specify ldpath as part of the qa step here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because Fedora and CentOS by default do not have /usr/local/lib64 in their search path: https://unix.stackexchange.com/questions/356624/why-isnt-usr-local-lib-on-the-library-path-by-default/357867#357867

There are of course other options how to make ld aware of this directoy, I simply did it this way to point out that it is a Fedora/CentOS specific thing. It could also be added to the docker.

Copy link
Member

Choose a reason for hiding this comment

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

How about setting PYTHONPATH (and LD_LIBRARY_PATH if needed) in the environment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also an option if that is preferred

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I was only thinking about LD_LIBRARY_PATH in the answer above. Why would you want to modify PYTHONPATH?

@mormj mormj requested review from mbr0wn and ryanvolz October 11, 2021 11:21
Copy link
Contributor

@ryanvolz ryanvolz left a comment

Choose a reason for hiding this comment

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

I think this is the best approach that will satisfy most users by ensuring the modules install to the right place in the path when the prefix is known and defaulting to where Python thinks they should go otherwise. I don't think it changes anything for packagers, and I'd guess almost all of them are already setting GR_PYTHON_DIR anyway.

@mormj
Copy link
Contributor

mormj commented Oct 11, 2021

This looks great - the logic of where to put things seems very sane. @schneider42 - when you get a chance can you rebase and squash?

Signed-off-by: schneider <schneider@blinkenlichts.net>
Installs GNURadio into the container and checks if Python can find,
import and use the gnuradio package.

Signed-off-by: schneider <schneider@blinkenlichts.net>
@schneider42
Copy link
Member Author

@mormj rebased and squashed into two different commits

@willcode willcode added Backport-3.9 Should be backported to 3.9 CI and removed Backport-3.9 Should be backported to 3.9 labels Oct 12, 2021
@mormj mormj merged commit 139be21 into gnuradio:master Oct 13, 2021
@willcode willcode added the Backport-3.9 Should be backported to 3.9 label Oct 13, 2021
@willcode willcode added ported-to-3.9 Has been ported to 3.9 and removed Backport-3.9 Should be backported to 3.9 labels Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI ported-to-3.9 Has been ported to 3.9
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants