InsightSoftwareConsortium / ITKModuleTemplate

A template to start an ITK Module
https://itk.org/ITKSoftwareGuide/html/Book1/ITKSoftwareGuide-Book1ch9.html#x50-1430009
Apache License 2.0
12 stars 16 forks source link

Add ITK runtime directory in PATH for Windows #94

Closed SimonRit closed 3 years ago

SimonRit commented 3 years ago

This PR adds ITK runtime directory in PATH for Windows. The commit 1ad4cfbe033c2016bbdceb511f4694746cac70d4 adresses the problem discussed here. It is not clear to me if this is a good solution, if this should be put in the cmd shell part of the yml file (I tried but it failed, see SimonRit/RTK@6373cb5a8ffa3f7d0a66912306a10e3ee07fa4f2), or if this should be addressed in RTK CMakeLists.txt... Let me know what you think. cc @LucasGandel

dzenanz commented 3 years ago

It looks like Python 3.9 is not yet available for Windows in GitHub's CI?

SimonRit commented 3 years ago

Indeed. If I understand things correctly, I think we would need to switch to windows-latest, see here. Should I modify the PR in this direction or drop the python 3.9 commit?

dzenanz commented 3 years ago

I prefer to keep the stable versions, meaning dropping 3.9 from the commit.

SimonRit commented 3 years ago

Done and mirrored the changes to {{cookiecutter.project_name}}/.github/workflows/build-test-package.yml. I'm still not sure 31c2c273bd18b0525a14541b1f57a86dde23a502 is the right thing to do!

dzenanz commented 3 years ago

Doing 31c2c27 in a project which requires DLLs (e.g. RTK) is fine, I am just not sure about module template as ITK builds static libraries by default. By I guess also having it is not a problem. If no one objects within a few days, merge.

jhlegarreta commented 3 years ago

Doing 31c2c27 in a project which requires DLLs (e.g. RTK) is fine, I am just not sure about module template as ITK builds static libraries by default. By I guess also having it is not a problem. If no one objects within a few days, merge.

I may not have a good understanding of the issue or if this is possible, but then can the addition be conditioned to the fact of building/having built ITK dynamically?

Otherwise, since the Python 3.9 build has been discarded for the moment, this would require updating the PR subject and message body.

SimonRit commented 3 years ago

Doing 31c2c27 in a project which requires DLLs (e.g. RTK) is fine, I am just not sure about module template as ITK builds static libraries by default. By I guess also having it is not a problem. If no one objects within a few days, merge.

I may not have a good understanding of the issue or if this is possible, but then can the addition be conditioned to the fact of building/having built ITK dynamically?

It could but the two yml files I have modified hard-code the compilation of ITK with BUILD_SHARED_LIBS ON. It is true, however, that it's useless for .github/workflows/build-test-package.yml because the module does not redefine CMAKE_OUTPUT_RUNTIME_DIRECTORY. It does not hurt though and I personally find it easier to keep the two files sync (now that I understood there are two files to maintain!).

Otherwise, since the Python 3.9 build has been discarded for the moment, this would require updating the PR subject and message body.

Done! Thanks for pointing it out.

SimonRit commented 3 years ago

As #95 was just speed-merged, can you rebase on top of master (and probably skip the first commit)? And another rename of the topic might be in order too

Done!

dzenanz commented 3 years ago

Let's merge if the builds come back clean.