Skip to content

Remove clean step when building module wheels on Linux#200

Merged
thewtex merged 1 commit intoInsightSoftwareConsortium:masterfrom
LucasGandel:manylinux-copy-configured-headers
Aug 26, 2022
Merged

Remove clean step when building module wheels on Linux#200
thewtex merged 1 commit intoInsightSoftwareConsortium:masterfrom
LucasGandel:manylinux-copy-configured-headers

Conversation

@LucasGandel
Copy link
Copy Markdown
Collaborator

@LucasGandel LucasGandel commented Aug 25, 2022

This is required when chaining module builds. The latest module to be built
must know about all the headers of former modules.

Our use case is that CudaCommon generates an export header that is then required when building RTK. I guess this does not happen for ITKUltrasound because all the modules it depends on are not generating export headers.

@dzenanz @thewtex Can you review this please?

@SimonRit FYI

@dzenanz dzenanz requested review from tbirdso and thewtex August 25, 2022 13:29
|| exit 1
# Copy generated headers into sources before cleaning
find _skbuild -type f -wholename "**/cmake-build/include/*" -print -exec cp {} include \;
${PYBIN}/python setup.py clean
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@LucasGandel thanks for the patch!

Can we just remove this line instead?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@thewtex Thanks, I just edited the change. You are right, it's better to remove the clean step and let the module CI decide what it wants to pick from this build.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@LucasGandel thanks!

Gives access to the configured headers of the module.
This is required when chaining module builds. The latest module to be built
must know about all the headers of former modules.
@LucasGandel LucasGandel force-pushed the manylinux-copy-configured-headers branch from d692766 to c953dc0 Compare August 26, 2022 07:20
@LucasGandel LucasGandel changed the title Copy generated headers to sources before cleaning module build on linux Remove clean step when building module wheels on Linux Aug 26, 2022
@thewtex thewtex merged commit 73b2230 into InsightSoftwareConsortium:master Aug 26, 2022
@LucasGandel LucasGandel deleted the manylinux-copy-configured-headers branch August 29, 2022 07:37
@LucasGandel
Copy link
Copy Markdown
Collaborator Author

Thanks a lot for the review! Should we release new ITKPythonBuilds for this change to be effective in modules CI?

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Sep 6, 2022

@LucasGandel thanks for the patch! Yes, a new ITKPythonBuilds-linux.tar.zst has been uploaded to the ITKPythonBuilds v5.3rc04.post2 with this change. Please test and open an new issue if any other improvements need to be made. Thank you!

@SimonRit
Copy link
Copy Markdown

SimonRit commented Sep 7, 2022

Great, thanks @thewtex! Looking at the assets, I can see that ITKPythonBuilds-linux.tar.zst has been updated but not ITKPythonBuilds-linux-manylinux2014.tar.zst. Was that intentional?

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Sep 9, 2022

Great, thanks @thewtex! Looking at the assets, I can see that ITKPythonBuilds-linux.tar.zst has been updated but not ITKPythonBuilds-linux-manylinux2014.tar.zst. Was that intentional?

Yes, we can update both if needed.

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.

5 participants