-
Notifications
You must be signed in to change notification settings - Fork 52
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
native itk dcmtk bumps #1209
native itk dcmtk bumps #1209
Conversation
thewtex
commented
Aug 23, 2024
•
edited
Loading
edited
- ci(python-wasm): disable dicom tests
- chore(native): bump ITK, DCMTK to match Docker
- build(native): add patch to fix ITKTotalVariation build on Linux
- build: re-use ITK, DCMTK source locations in native, docker builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Let me know if you'd like me to try the build on native windows.
LGTM.
itk_wasm_env.bash
Outdated
@@ -5,16 +5,16 @@ function die() { | |||
exit 1 | |||
} | |||
|
|||
if [$OSTYPE == "cygwin"] || [$OSTYPE == "msys"] || [$OSTYPE =="win32"]; then | |||
if test "$OSTYPE" = "cygwin" || test "$OSTYPE" = "msys" || test "$OSTYPE" = "win32"; then | |||
echo "Windows platform detected ... adding \"/Zc:__cplusplus /DNOMINMAX\" to \$CXXFLAGS" | |||
export CXXFLAGS="/Zc:__cplusplus /DNOMINMAX" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My current script shows, but can't remember why I updated it. I will figure it out and post here again:
export CXXFLAGS="/Zc:__cplusplus /Zc:preprocessor /DNOMINMAX"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or I can just update it in a different PR if /Zc:preprocessor
is a legit requirement for MSVC native builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jadh4v thanks! I added that flag.
I am going to merge to rebase a different branch on this, but if you test and find any issues with MSVC, another patch would be appreciated.
@@ -102,4 +102,4 @@ packages/transform-io/typescript/demo-app/ | |||
*.egg-info | |||
|
|||
native/ | |||
|
|||
src/docker/itk-wasm-base/itk_wasm_env_vars.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why gitignore though? we don't expect commits often but expect local changes to the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a derived file from ITK-Wasm/itk_wasm_env.bash. We need to add in there since Docker's build context only takes into account the local directory.
Co-authored-by: Shreeraj Jadhav <shreerajjadhav@gmail.com>
d07d12b
into
InsightSoftwareConsortium:main