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

test: make integration tests cross-platform #203

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

TomMelt
Copy link
Member

@TomMelt TomMelt commented Dec 9, 2024

this makes the integration tests more portable.

  • change permissions on python files so they are no longer executables
  • remove python shebang from said python files
  • update CMakeLists.txt to require python interpreter (for building tests only) and use Python_EXECUTABLE which is set by find_package (Python COMPONENTS Interpreter REQUIRED)

@TomMelt TomMelt added the enhancement New feature or request label Dec 9, 2024
@TomMelt TomMelt self-assigned this Dec 9, 2024
@TomMelt TomMelt force-pushed the cross-platform-tests branch from ab707c6 to 2521e29 Compare December 9, 2024 11:52
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

Seems sensible.

So when searching for python and implicitly setting Python_EXECUTABLE this does now work OK on Windows?
Since we don't set this now as far as I can see.

GitHub flags the python scripts as changed, but then shows no lines changed listing it as an 'empty file'. Is this because it interprets the removal of a shebang in a strange way?

@TomMelt TomMelt mentioned this pull request Dec 9, 2024
@TomMelt
Copy link
Member Author

TomMelt commented Dec 9, 2024

Seems sensible.

So when searching for python and implicitly setting Python_EXECUTABLE this does now work OK on Windows? Since we don't set this now as far as I can see.

This is set by find_package (Python COMPONENTS Interpreter REQUIRED) as mentioned in the PR description. The linux tests are failing but will be fixed by my next commit. It is just picking up the wrong environment.

GitHub flags the python scripts as changed, but then shows no lines changed listing it as an 'empty file'. Is this because it interprets the removal of a shebang in a strange way?

No, it because the permissions were changed from executable (755) to read/write (644). There is no way for github to display that info. It's a bit like when you rename a file. There are zero changes to the file contents but the git metadata changes.

@TomMelt TomMelt force-pushed the cross-platform-tests branch from 2521e29 to acb216d Compare December 9, 2024 12:19
@jatkinson1000
Copy link
Member

@TomMelt OK, so are we leaving the shebangs in the files?
It's just there is a ticked off box saying they have been removed at the top, so I expected to see this in the file diff at the same time as changing the permissions.

@TomMelt TomMelt force-pushed the cross-platform-tests branch from f1c00fe to 5b79f59 Compare December 9, 2024 15:12
@TomMelt
Copy link
Member Author

TomMelt commented Dec 9, 2024

@TomMelt OK, so are we leaving the shebangs in the files? It's just there is a ticked off box saying they have been removed at the top, so I expected to see this in the file diff at the same time as changing the permissions.

my bad... now I understand you. I hadn't pushed that commit yet 🤦

You can now see the shebang removal 👌

@TomMelt TomMelt force-pushed the cross-platform-tests branch from 5b79f59 to 966fbd4 Compare December 9, 2024 15:13
@TomMelt TomMelt merged commit fe20cbf into windows-ci-build Dec 9, 2024
4 checks passed
@TomMelt TomMelt deleted the cross-platform-tests branch December 9, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants