Skip to content

Add GHA and several test fixes #2321

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 6 commits into from
Jul 23, 2020
Merged

Add GHA and several test fixes #2321

merged 6 commits into from
Jul 23, 2020

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Jul 23, 2020

This adds a basic GHA test suite. Features:

  • macOS, Windows, and Linux, all versions of Python, default C++ standard.

Test related fixes:

  • Fix for PyPy2 tests not passing
  • CMake passthrough fix for cmake test, fixes Build issue on max OS #2295 (will possibly improve later during the CMake cleanup)
  • Fix warning for Python 2 + C++14+ (will possibly improve later during the CMake cleanup and help user code - test only for now)
  • IDE-based build systems (MSVC) were not able to run the embed test - fixed.
  • Adding a virtual environment directory with env in the name is now ignored.
  • Fixed Python 3.9+ warning (upstream, see bpo-41366: Fix clang warning for sign conversion python/cpython#21592 )

Future plans:

  • Fix the Windows Python 3.8+ builds
  • Add multiple C++ version checks
  • Add docker-based checks too, for CUDA & LLVM versions
  • Add a -DDOWNLOAD_EIGEN for CMake, add Eigen testing here.
  • Add docs check and other specialty builds too
  • If the matrix gets too big, maybe split up.
  • Once this has fairly good overlap with old CI, start dropping old CI.

This should be "rebased & merged".

@@ -37,3 +37,4 @@ MANIFEST
sosize-*.txt
pybind11Config*.cmake
pybind11Targets.cmake
/*env*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the leading slash intentional?

Copy link
Collaborator Author

@henryiii henryiii Jul 23, 2020

Choose a reason for hiding this comment

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

Yes, I'm assuming a virtual environment would be in the main directory or in a build directory (which should be already ignored)

@@ -203,10 +211,10 @@ foreach(target ${test_targets})

# Always write the output file directly into the 'tests' directory (even on MSVC)
if(NOT CMAKE_LIBRARY_OUTPUT_DIRECTORY)
set_target_properties(${target} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${testdir})
set_target_properties(${target} PROPERTIES LIBRARY_OUTPUT_DIRECTORY "${testdir}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought cmake doesn't care about quotes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, without quotes spaces turn into separate arguments. All filename variables need to have quotes. We aren't testing with a space in the name of a directory, but I expect this would fail without quotes.

@henryiii
Copy link
Collaborator Author

henryiii commented Jul 23, 2020

PyPy3 on macOS may have an error I didn't see before, due to not having numpy installed when I first ran the tests. I'll try adding the extra index recommended for PyPy wheels, maybe if we don't compile them, it will be both faster and work properly. Edit: This will only help Linux, not macOS, AFAICT.

Adds requirements file too.
@henryiii
Copy link
Collaborator Author

I've dropped NumPy on macOS + PyPy3 for now - looks like their issue (it's failing on import numpy). I've trimmed down the number of jobs a bit - we do 2.7, 3.5, and 3.8 on all OS's, and 3.6 and 3.7 on a select few. A similar trick could work for C++ versions.

@henryiii henryiii merged commit b16347e into master Jul 23, 2020
@henryiii henryiii deleted the ci/gha4a branch July 23, 2020 21:42
@henryiii henryiii added the ci related to the CI system label Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci related to the CI system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build issue on max OS
2 participants