-
Notifications
You must be signed in to change notification settings - Fork 45
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
Install compiled libraries only to 'lib' #195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm not at all sure about this, but while looking this over I noticed something. That is the fact that if we remove the lines you are proposing to remove, then there is no reason to pass _build_type
into these two macros anymore.
In turn, that leads me to look at where the macros are being called. They are called once with an "empty" (I assume "Release") build type for all platforms. And then, only on WIN32, they are called once for each build type. I don't exactly know what that means.
Looking at the history of this code, it was added in 2016 (via 1d7b4eb before we used PRs) and 2018 (via #10). So this code has been there quite a while.
I think we need to do some more extensive testing on this besides the normal CI. In particular, we should also test this out with Windows Debug to see what happens. If we can get it to work there, then I'm fine with the change, but we should then also remove the build_type
argument from both macros (and maybe get rid of the macros completely).
Bump. Any updates or plan for this? |
Yep:
By all means, if you're interested in seeing this merged, please test it and provide feedback. |
The current behavior results in duplicate libraries installed to both the Python module in libdir and also into the global lib directory for the prefix. The former seems unnecessary, so we should install the libraries only to lib. Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
277ef2e
to
c5f3de2
Compare
All right, the extensive CI is actually really happy with this. So I'm going to go ahead and merge this in, since it seems to be an improvement, and doesn't seem to have downsides. After a few days in source, I'll likely also do a release of this just to be sure the buildfarm is happy with it as well. |
There seems that some regression might have happened after #195. When removing those 2 lines, we avoid to install the .so files in lib *and* python path. Signed-off-by: Matthias Schoepfer <m.schoepfer@rethinkrobotics.com>
The current behavior results in duplicate libraries installed to both the Python module in libdir and also into the global lib directory for the prefix.
The former seems unnecessary, so we should install the libraries only to lib.