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.

Sign up for GitHub

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

Jump to bottom

BUG: Pass cmake-options to Windows script for Python module #58

Merged
merged 1 commit into from
Feb 24, 2023
Merged

BUG: Pass cmake-options to Windows script for Python module #58

merged 1 commit into from
Feb 24, 2023

Conversation

SimonRit
Copy link

CMake options were not passed to the Windows script for Python packaging. MacOS and Linux scripts work adequately.

The '-DRTK_BUILD_APPLICATIONS:BOOL=OFF` option should turn off the compilation of applications using gengetopt and a simple search for ggo in the raw logs shows that it is the case for
https://github.com/RTKConsortium/RTK/actions/runs/4222316866/jobs/7330680579
and not before this commit
https://github.com/RTKConsortium/RTK/actions/runs/4201859054/jobs/7295273280.

@dzenanz dzenanz requested a review from tbirdso February 20, 2023 13:41
@SimonRit
Copy link
Author

I think there is a problem with InsightSoftwareConsortium/ITKPythonPackage@2dca3c8. In this build, the two cmake options -DRTK_BUILD_APPLICATIONS:BOOL=OFF -DRTK_CUDA_VERSION=11.6 are processed as one string. I'll try to find a fix, do not merge yet...

@SimonRit SimonRit changed the title BUG: Pass cmake-options to Windows script for Python module WIP: Pass cmake-options to Windows script for Python module Feb 20, 2023
@SimonRit SimonRit changed the title WIP: Pass cmake-options to Windows script for Python module BUG: Pass cmake-options to Windows script for Python module Feb 21, 2023
@SimonRit
Copy link
Author

I think there is a problem with InsightSoftwareConsortium/ITKPythonPackage@2dca3c8. In this build, the two cmake options -DRTK_BUILD_APPLICATIONS:BOOL=OFF -DRTK_CUDA_VERSION=11.6 are processed as one string. I'll try to find a fix, do not merge yet...

This is fixed with double quotes (in a single quote string): https://github.com/RTKConsortium/RTK/compare/f6473ee2ee05e9404597863e17259edd39e85132..e8f0874585dcea01f89a520b765205b7466f599c.

@SimonRit SimonRit mentioned this pull request Feb 21, 2023
Open
@tbirdso
Copy link
Collaborator

tbirdso commented Feb 21, 2023

@SimonRit Thanks for working on this!

Does adding extra quotes around cmake-options parameters impact processing for Linux and macOS builds? Do you have a link to where the changes proposed in https://github.com/RTKConsortium/RTK/compare/f6473ee2ee05e9404597863e17259edd39e85132..e8f0874585dcea01f89a520b765205b7466f599c succeed across platforms?

@SimonRit
Copy link
Author

@SimonRit Thanks for working on this!

Does adding extra quotes around cmake-options parameters impact processing for Linux and macOS builds? Do you have a link to where the changes proposed in https://github.com/RTKConsortium/RTK/compare/f6473ee2ee05e9404597863e17259edd39e85132..e8f0874585dcea01f89a520b765205b7466f599c succeed across platforms?

No... I'm not testing MacOS but it fails on Linux:
https://github.com/RTKConsortium/RTK/actions/runs/4229904801/jobs/7356304475#step:4:496
I'll work on a fix today...

@SimonRit SimonRit marked this pull request as draft February 22, 2023 09:31
@SimonRit SimonRit force-pushed the cmake_options_win branch 3 times, most recently from 16378a5 to 9ab9aa5 Compare February 22, 2023 14:48
@SimonRit
Copy link
Author

The PR will be ready for review after a small modification (ITKPythonPackage hash and repo) when InsightSoftwareConsortium/ITKPythonPackage#252 is merged.

@SimonRit SimonRit marked this pull request as ready for review February 23, 2023 09:02
@SimonRit
Copy link
Author

This is ready to be merged in my opinion...

tbirdso
tbirdso approved these changes Feb 24, 2023
View reviewed changes
Copy link
Collaborator

@tbirdso tbirdso left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great!

EDIT: xref passing jobs in InsightSoftwareConsortium/ITKPythonPackage#252

@tbirdso tbirdso merged commit f2191a0 into InsightSoftwareConsortium:main Feb 24, 2023
@SimonRit SimonRit deleted the cmake_options_win branch February 24, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzenanz dzenanz dzenanz approved these changes

@tbirdso tbirdso tbirdso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SimonRit @tbirdso @dzenanz