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

ENH: Add option on using system library #1020

Merged
merged 1 commit into from
Jan 24, 2022
Merged

ENH: Add option on using system library #1020

merged 1 commit into from
Jan 24, 2022

Conversation

alanliu02
Copy link
Contributor

Tested on Windows10, MSVC16, VS2019.
There are some issues with DCMTK. When compling DCMTK related plugins, it is required to put compiled libraries in some project folder to be able to continue compling.

@alanliu02
Copy link
Contributor Author

Solving issues like #911

Copy link
Member

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

We have been using CTK with VTK, ITK, etc. built outside of CTK for many years in 3D Slicer, as you can specify these variables as command-line arguments. So, adding the options is not necessary. However, it does not hurt to add these options. Maybe you could mark them as "advanced" to prevent cluttering the CMake GUI.

@alanliu02
Copy link
Contributor Author

alanliu02 commented Jan 22, 2022

We have been using CTK with VTK, ITK, etc. built outside of CTK for many years in 3D Slicer, as you can specify these variables as command-line arguments. So, adding the options is not necessary. However, it does not hurt to add these options. Maybe you could mark them as "advanced" to prevent cluttering the CMake GUI.

@lassoan Thanks for the hint, I think it's necessery to let advanced user to know that this option is available.
Since the Internet connection can sometimes be wrong, one will need this option to prevent building CTK with other thirdparty libraries online, instead they can chose to compile thirdparty libraries first with local source and let CMake find exist libraries.
I now marked these options as advanced and I think it fits your mind. Thanks for the review!

Copy link
Member

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Thank you, it looks good to me.

@lassoan
Copy link
Member

lassoan commented Jan 22, 2022

If you squash the commits into one (and make sure the commit comment is valid) then it should be good to go.

@alanliu02
Copy link
Contributor Author

I think it's ready to merge @lassoan @pieper

Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

LGTM

@lassoan lassoan merged commit 4a743e2 into commontk:master Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants