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

Sanitize FindPythonLibsNew.cmake #1879

Closed
wants to merge 3 commits into from

Conversation

tvatter
Copy link

@tvatter tvatter commented Aug 13, 2019

This PR fixes pybind/cmake_example#24

@wjakob
Copy link
Member

wjakob commented Aug 13, 2019

For posterity, can you explain in more detail here what the actual problem is?

I'm not 100% happy with this PR yet: I found the print() statements much easier to understand than the big multi-line statement. Also, the script definitely shouldn't create files on disk ("trash").

@tvatter
Copy link
Author

tvatter commented Aug 13, 2019

In a conda environment with Python 3.7.4 and a bunch of packages installed, I noticed that the first call to get_config_var produces "garbage" output, which then messes with the rest of the script.

By capturing the unwanted output when calling the functions and printing afterwards, we prevent something like this from happening.

Also, ';'.join([str(item) for item in values]) directly converts into the list format from cmake, which makes lines 108 and 109 unnecessary.

Regarding the creation of files on disk, I'm pretty new to Python, but I can have a look at another way of "sinking" the unwanted output somewhere.

@tvatter
Copy link
Author

tvatter commented Aug 13, 2019

For the creation of files on disk, would a replace of trash by os.devnull be OK?

@tvatter
Copy link
Author

tvatter commented Aug 13, 2019

Regarding the multiple print vs how it is done in the PR: now the code conveys exactly what it is doing, namely (a) creating a list of values, (b) converting the Python list to cmake format, (c) outputing the list.

@wjakob
Copy link
Member

wjakob commented Aug 13, 2019

Does os.devnull work on all platforms (Windows?)

@tvatter
Copy link
Author

tvatter commented Aug 13, 2019

It is supposed to.

@wjakob
Copy link
Member

wjakob commented Aug 15, 2019

Ok, I see. Could you add a comment saying that one of the commands prints output to stdout just so that the context is clear? Please also fix the CI issues.

@wjakob
Copy link
Member

wjakob commented Aug 19, 2019

Just for clarification: if you look through the CI results, you'll see that many tests now fail with errors (ImportError: No module named pybind11_tests) which I assume is a side effect of the changes you have made here.

@tvatter
Copy link
Author

tvatter commented Aug 19, 2019

I guess too, but I have no idea what makes it fail on Python 2.7 (Linux/OSX) and Windows, as the changes are completely straightforward. I will have a look today.

@wjakob
Copy link
Member

wjakob commented Sep 4, 2019

ping? :)

@tvatter
Copy link
Author

tvatter commented Sep 4, 2019

Last week, I tried to figure out what was going on:

https://travis-ci.org/pybind/pybind11/jobs/571536066#L1049

The issue is that I cannot reproduce locally, as it works with Python 2.7 for me, and I have no windows computer. I will borrow my wife's computer tonight to figure out the windows thing, but I really don't get what's happening on ubuntu since I have a very similar config :/

@wjakob
Copy link
Member

wjakob commented Sep 4, 2019

Somehow the suffix of the python extension library gets corrupted. It is called pybind11_testsNone, so it should not be a surprise that the python test suite is unable to import it.

@wjakob
Copy link
Member

wjakob commented Sep 19, 2019

Dear @tvatter,

I was wondering if there were any news here? Apart from the fix, I was also wondering if you could provide some more information about the circumstances where this problem appears?

I could not the spurious output produced by s.get_config_var on Anaconda with Python 3.7 on MacOS.

Best,
Wenzel

@tvatter
Copy link
Author

tvatter commented Sep 19, 2019

I'm so sorry, I've not been to actually reproduce the issue, and I've been completely swamped by teaching since the semester started. If having this open bothers you, don't hesitate to close and I'll come back to this whenever I have more bandwidth available. Apologies.

@wjakob
Copy link
Member

wjakob commented Sep 20, 2019

Ok, I'll clear this for now then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python config failure from FindPythonLibsNew.cmake
2 participants