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

[zlib] Ignore external ZLIB_DLL #26885

Closed
wants to merge 13 commits into from
Closed

[zlib] Ignore external ZLIB_DLL #26885

wants to merge 13 commits into from

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Sep 20, 2022

  • What does your PR fix?

    Hardcodes the triplet linkage into the installed headers, so that usage no longer depends on external definition of ZLIB_DLL.
    For example, GDAL always applies ZLIB_DLL. Noticed in log from [gdal] Update to v3.5.2 #26676 (comment):

    LINK : warning LNK4286: symbol 'crc32' defined in 'zlib.lib(crc32.obj)' is imported by 'gdal.lib(cpl_minizip_unzip.cpp.obj)'
    LINK : warning LNK4217: symbol 'deflateInit_' defined in 'zlib.lib(deflate.obj)' is imported by 'gdal.lib(cpl_vsil_gzip.cpp.obj)' in function 'CPLZLibDeflate'
    

    Given the broad impact of port zlib changes, this PR should be considered for combination with other zlib changes:
    [zlib] Fix CVE-2022-37434 #26792

    Follow-up changes:

    • Update minizip to the same source version as zlib.
    • Revise libkml, triggered from zlib/minizip issues.
  • Which triplets are supported/not supported? Have you updated the CI baseline?

    unchanged, no

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    yes

github-actions[bot]
github-actions bot previously approved these changes Sep 20, 2022
@FrankXie05 FrankXie05 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Sep 20, 2022
@Neumann-A
Copy link
Contributor

Why has this not been a problem before?

@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 21, 2022

Why has this not been a problem before?

a) Packages tend to unconditionally set ZLIB_DLL.
Evidence: gdal as mentioned aboved; git grep ZLIB_DLL
b) There is only a warning in static builds, not an error.
c) Contributors and reviewers don't care for this warning. In particular when connected with other errors
Evidence: the mentioned gdal issue; https://github.com/microsoft/vcpkg/search?q=LNK4217&type=issues

@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 21, 2022

libkml probably stumbles over a mix of headers from zlib and its vendored copy of minizip.

github-actions[bot]
github-actions bot previously approved these changes Sep 21, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one portfile where deprecated functions are used.

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

  • ports/libkml/portfile.cmake

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/minizip/vcpkg.json

Valid values for the license field can be found in the documentation

github-actions[bot]
github-actions bot previously approved these changes Sep 22, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/minizip/vcpkg.json

Valid values for the license field can be found in the documentation

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout d52632d9a0c4a5b57de285b1e03722a067dd80eb -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 6e8968a..8edb36d 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -3834,7 +3834,7 @@
     },
     "libkml": {
       "baseline": "1.3.0",
-      "port-version": 8
+      "port-version": 9
     },
     "liblas": {
       "baseline": "1.8.1",
diff --git a/versions/l-/libkml.json b/versions/l-/libkml.json
index 8b24a12..7b37667 100644
--- a/versions/l-/libkml.json
+++ b/versions/l-/libkml.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "b251016911a5eeae287e3a5bd67a30e3213b2b8e",
+      "version": "1.3.0",
+      "port-version": 9
+    },
     {
       "git-tree": "1ce9dd050baf9ae42da2bb0c6d0feae1241b6e01",
       "version-string": "1.3.0",

github-actions[bot]
github-actions bot previously approved these changes Sep 22, 2022
@dg0yt dg0yt marked this pull request as ready for review September 22, 2022 07:22
FrankXie05
FrankXie05 previously approved these changes Sep 23, 2022
@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Sep 23, 2022
@dg0yt dg0yt marked this pull request as draft September 23, 2022 17:18
@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 23, 2022

I want to improve the libkml mingw patch.

github-actions[bot]
github-actions bot previously approved these changes Sep 23, 2022
@FrankXie05 FrankXie05 removed the info:reviewed Pull Request changes follow basic guidelines label Sep 26, 2022
@dg0yt dg0yt marked this pull request as ready for review October 1, 2022 05:18
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 5f144173006dbeea7b9cb017775675606666a207 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 9f9b357..0c8358f 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -3838,7 +3838,7 @@
     },
     "libkml": {
       "baseline": "1.3.0",
-      "port-version": 8
+      "port-version": 9
     },
     "liblas": {
       "baseline": "1.8.1",
@@ -4769,8 +4769,8 @@
       "port-version": 1
     },
     "minizip": {
-      "baseline": "1.2.11",
-      "port-version": 11
+      "baseline": "1.2.12",
+      "port-version": 0
     },
     "minizip-ng": {
       "baseline": "3.0.5",
diff --git a/versions/l-/libkml.json b/versions/l-/libkml.json
index 8b24a12..5797ff9 100644
--- a/versions/l-/libkml.json
+++ b/versions/l-/libkml.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "9941e66029546372544fed13066e9a9756981dd3",
+      "version": "1.3.0",
+      "port-version": 9
+    },
     {
       "git-tree": "1ce9dd050baf9ae42da2bb0c6d0feae1241b6e01",
       "version-string": "1.3.0",
diff --git a/versions/m-/minizip.json b/versions/m-/minizip.json
index 9c1239b..c85a7b5 100644
--- a/versions/m-/minizip.json
+++ b/versions/m-/minizip.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "65a32bfac58033a215e271db94d5e022fce816e3",
+      "version-semver": "1.2.12",
+      "port-version": 0
+    },
     {
       "git-tree": "528703ed8d2b78aeaa55695765535efafa24540b",
       "version-semver": "1.2.11",

@dg0yt dg0yt requested a review from FrankXie05 October 5, 2022 06:07
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I love this change. Are you saying you want me to cherry pick it into #27226 or vice versa?

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 19, 2022

Please do the cherry-pick.

@BillyONeal
Copy link
Member

Thanks again!

@dg0yt dg0yt deleted the zlib-dll branch October 28, 2022 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants