Skip to content

tests: avoid putting build products into source directory #2353

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

Merged
merged 4 commits into from
Aug 19, 2020

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Aug 1, 2020

As @rwgk has noted, and as sometimes comes up on the Gitter, pybind11 doesn't respect build and source separation - it dumps the test binaries into the source directories, meaning you can't have multiple build directories at once; or, at least, you have to recompile each time you want to change directories (which eliminates the main reason to have build directories in the first place). This does some cleanup, and makes sure that the source directories do not get artifacts! Only one Python file is copied in; the rest is done via PyTest config.

Also added a warning if someone tries an in-place build, which is a bad idea in general, and should be avoided.

  • All build products go in the binary dir
    • PyTest's ini file solves the old issue that required this ugly hack
  • Warns if in place builds are used (but they are still fully supported & tested)
  • Warns if separate build directory is being used but output files have already polluted the source directory
  • Drops the continue-on-error attempt in GHA, didn't do anything.
  • If build products are in the source (in place build or leftover), you can run pytest directly on Python 2 (was broken before)

@henryiii henryiii marked this pull request as draft August 1, 2020 04:55
@henryiii henryiii force-pushed the tests/clean branch 3 times, most recently from a18c888 to 4346b25 Compare August 2, 2020 02:42
@henryiii
Copy link
Collaborator Author

henryiii commented Aug 3, 2020

Current problems: Everything works, except on Windows, where I get:

Python 2.7:

Failed to import pybind11_tests from pytest:
   ImportError: AttributeError: gil_scoped

Python 3.5 and 3.6:

import pybind11_tests  # noqa: F401 imported but unused
  ImportError: SystemError: <class 'property'> returned a result with an error set

Python 3.7 is fine. (I'm also taking advantage of this to test a few extra configurations, but not related to the actual PR)

@henryiii henryiii force-pushed the tests/clean branch 3 times, most recently from 64905ff to cbe4a90 Compare August 5, 2020 21:26
@henryiii
Copy link
Collaborator Author

henryiii commented Aug 5, 2020

Further debugging: There’s a hack in the conftest.py that hides the real errors on importing. Removing the hack gives clearer errors:

Python 2.7 can’t import anything from pybind11_tests . ImportError: cannot import name gil_scoped and such for each submodule.

Python 3.5 and 3.6 produce ImportError: generic_type: type "ConstructorStats" is already registered! every time.
And of course, all other OSs, and other Pythons, work fine. And building in place works fine, even on Windows + Python 2.7, 3.5, and 3.6.


Python 2.7:

 _____________________ ERROR collecting test_gil_scoped.py _____________________
  ImportError while importing test module 'D:\a\pybind11\pybind11\tests\test_gil_scoped.py'.
  Hint: make sure your test modules/packages have valid Python names.
  Traceback:
  ..\..\tests\test_gil_scoped.py:4: in <module>
      from pybind11_tests import gil_scoped as m
  E   ImportError: cannot import name gil_scoped
  ______________________ ERROR collecting test_iostream.py ______________________
  ImportError while importing test module 'D:\a\pybind11\pybind11\tests\test_iostream.py'.
  Hint: make sure your test modules/packages have valid Python names.
  Traceback:
  ..\..\tests\test_iostream.py:2: in <module>
      from pybind11_tests import iostream as m
  E   ImportError: cannot import name iostream
...

Python 3.5 and 3.6:

_______________________ ERROR collecting test_async.py ________________________
  ImportError while importing test module 'D:\a\pybind11\pybind11\tests\test_async.py'.
  Hint: make sure your test modules/packages have valid Python names.
  Traceback:
  ..\..\tests\test_async.py:4: in <module>
      from pybind11_tests import async_module as m
  E   ImportError: generic_type: type "ConstructorStats" is already registered!
  ______________________ ERROR collecting test_buffers.py _______________________
  ImportError while importing test module 'D:\a\pybind11\pybind11\tests\test_buffers.py'.
  Hint: make sure your test modules/packages have valid Python names.
  Traceback:
  ..\..\tests\test_buffers.py:8: in <module>
      from pybind11_tests import buffers as m
  E   ImportError: generic_type: type "ConstructorStats" is already registered!
...

@henryiii henryiii force-pushed the tests/clean branch 5 times, most recently from e8fbe72 to e98c607 Compare August 6, 2020 03:01
@henryiii henryiii force-pushed the tests/clean branch 2 times, most recently from d47a8db to 4b7aa3c Compare August 17, 2020 05:10
@henryiii henryiii marked this pull request as ready for review August 17, 2020 05:40
@henryiii
Copy link
Collaborator Author

I'm not totally sure what I fixed to make this start working, but it's working perfectly now after rebasing on my latest fixes.

@henryiii
Copy link
Collaborator Author

I've added a warning if the source directory is dirty with previous output files (the first time you build with this, you'll likely have a dirty source directory from previous runs).

@wjakob
Copy link
Member

wjakob commented Aug 19, 2020

Hi @Henriii,

supporting proper out-of-tree builds sounds great to me, and those changes all look good to me. I would consider it important that no warning is generated when pybind11 is used (via add_subdirectory) by another project, and that user/project chooses to do an in-tree build. (But my understanding from reading the code is that no warning is generated in that case)

An aside question: do you know what CMake's policy regarding in-tree builds is? I vaguely remember seeing a deprecation warning at some point, which seemed a bit pushy (In-tree builds are perfectly OK in some cases)

Thanks,
Wenzel

@henryiii
Copy link
Collaborator Author

henryiii commented Aug 19, 2020

But my understanding from reading the code is that no warning is generated in that case

Yes, this is only for when this is the main project. As a subproject, it's up to the implementer of that project, not us. We didn't add anything to the source directory in those cases, so no change there :) (just our own testing)

do you know what CMake's policy regarding in-tree builds is

As far as I know, it will always be 100% up to the package authors. Many packages do manually disable in-source builds, or generate warnings, but there are some cases where it's easier/simpler, and CMake itself isn't going to stop supporting those.

@henryiii henryiii merged commit 04fdc44 into pybind:master Aug 19, 2020
@henryiii henryiii deleted the tests/clean branch August 19, 2020 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants