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

[gdcm2] fix improper cmake file setup #4880

Merged
merged 4 commits into from
Dec 11, 2018
Merged

[gdcm2] fix improper cmake file setup #4880

merged 4 commits into from
Dec 11, 2018

Conversation

twMr7
Copy link
Contributor

@twMr7 twMr7 commented Dec 2, 2018

move cmake files to proper location, and modify path in contents accordingly

@twMr7 twMr7 mentioned this pull request Dec 2, 2018
4 tasks
@@ -22,7 +21,7 @@ vcpkg_configure_cmake(
-DGDCM_INSTALL_INCLUDE_DIR=include
-DGDCM_USE_SYSTEM_EXPAT=ON
-DGDCM_USE_SYSTEM_ZLIB=ON
${ADDITIONAL_OPTIONS}
-DGDCM_USE_SYSTEM_OPENJPEG=OFF
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this option is wrong, unless really necessary we should use system jpeg (and adding it to the "build-depends" field in the control file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this explicit OFF by intention. It still is a workaround for system open-jpeg cannot be found properly from gdcm2. Since gdcm2 provide its own bundle, I think it is still considered a "working" port, unless the bundled open-jpeg is not working as I assumed.

Copy link
Contributor

Choose a reason for hiding this comment

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

what is the problem in finding the system jpeg? Something similar happens in other ports, usually it is just their "FindJPEG" that has to be patched (or fully removed so that the system one is used, which is supposed to work). Providing twice the same library (in case the user already has an OpenJPEG implementation installed) can bring in many problem for downstream projects

@twMr7
Copy link
Contributor Author

twMr7 commented Dec 3, 2018

Ok @cenit , check out the new commit to see if this patch work. I tested that on x64-windows and x64-windows-static builds only.

@ras0219-msft ras0219-msft self-assigned this Dec 11, 2018
@ras0219-msft ras0219-msft merged commit 9b8cc80 into microsoft:master Dec 11, 2018
@ras0219-msft
Copy link
Contributor

ras0219-msft commented Dec 11, 2018

Thanks for the PR! (and @cenit for the review 👍 )

While merging, I replaced the cmake-file patching with vcpkg_fixup_cmake_targets() which does equivalent work in a smaller space :)

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.

3 participants