[openssl] Finish unified configuration for windows and non-windows by dg0yt · Pull Request #27261 · microsoft/vcpkg

dg0yt · 2022-10-16T14:31:52Z

What does your PR fix?

Complements [openssl] Revise jom/nmake support #27150 for the unix part, using a similar structure.
Leverages vcpkg_configure_make/vcpkg_install_make via a simple configure wrapper script instead of adding indirection via a complex CMake project.
Unifies windows and unix feature configuration in the main port file, ensuring consistency. (Inconsistencies was added even by those who should review PRs: [openssl] Add no-ssl3 and no-weak-ssl-ciphers to Configure options #28743)
Retains etc/ssl.
Removes pointless no-ssl2 option which was deprecated years ago.
Improves the linux kernel headers message.
Adds a check and message for Perl's IPC::Cmd module. Cf. [openssl] Build error #27929 et al.
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
Tests

This change would benefit from independent testing wrt to all the target triplet particularities.
- x64-linux (CI, @dg0yt)
- x64-linux-dynamic (@dg0yt)
- arm-linux cross (@dg0yt)
- x64-mingw-static cross (@dg0yt)
- x64-mingw-static native (@dg0yt)
- x64-mingw-dynamic cross (@dg0yt)
- x64-mingw-dynamic native (@dg0yt)
- x64-osx (CI)
- *-android
- *-ios
- *-emscripten

github-actions

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 79fa9d85cb4510b05c2657bd7ec8a6a2cb266dc5 -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/baseline.json b/versions/baseline.json
index 888ca87..0fe514b 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -5474,7 +5474,7 @@
     },
     "openssl": {
       "baseline": "3.0.5",
-      "port-version": 5
+      "port-version": 6
     },
     "openssl-unix": {
       "baseline": "1.1.1h",
diff --git a/versions/o-/openssl.json b/versions/o-/openssl.json
index 58def96..e33a631 100644
--- a/versions/o-/openssl.json
+++ b/versions/o-/openssl.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "505abd97c0b1f95c72b96509bd13fa7d0dc84225",
+      "version": "3.0.5",
+      "port-version": 6
+    },
     {
       "git-tree": "067a790dfd1559e77d5a199ccbe982322882d667",
       "version": "3.0.5",

dg0yt · 2023-01-18T07:10:36Z

~~Baseline~~ regression with similarities to #28827, investigating.

openssl/openssl@e80381e

dg0yt · 2023-02-16T08:05:49Z

CC @BillyONeal This should really be handled before the next upstream update.

JavierMatosD

Everything looks great, with some minor changes/feedback. The important one is that it looks like you are using {VERSION} openssl/portfile.cmake but removed vcpkg_minimum_required.

Other than that, it looks good to me. :)

JavierMatosD · 2023-02-23T22:44:08Z

+    file(COPY "${CURRENT_PACKAGES_DIR}/${script}" DESTINATION "${CURRENT_PACKAGES_DIR}/tools/${PORT}")
+    file(REMOVE "${CURRENT_PACKAGES_DIR}/${script}" "${CURRENT_PACKAGES_DIR}/debug/${script}")
 endforeach()
+vcpkg_copy_tools(TOOL_NAMES openssl AUTO_CLEAN)


c_rehash could be added to vcpkg_copy_tools

I cannot handle scripts with vcpkg_copy_tools because it only handles .exe for windows. And this code runs for native mingw builds.

JavierMatosD · 2023-02-23T22:44:23Z

+file(MAKE_DIRECTORY "${CURRENT_PACKAGES_DIR}/tools/${PORT}")
+foreach(script IN ITEMS "bin/c_rehash")
+    file(COPY "${CURRENT_PACKAGES_DIR}/${script}" DESTINATION "${CURRENT_PACKAGES_DIR}/tools/${PORT}")
+    file(REMOVE "${CURRENT_PACKAGES_DIR}/${script}" "${CURRENT_PACKAGES_DIR}/debug/${script}")


Why not just rename instead of copy and remove?

It doesn't matter that much. We always have to remove the debug variant. So there will always be two commands.
The loop is obsolete now. OTOH it makes clear that c_rehash is a script. This fact is relevant with regard to vcpkg_copy_tools.

JavierMatosD · 2023-02-23T22:48:29Z

+if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic" AND VCPKG_TARGET_IS_MINGW)
+    # For consistency with nmake build
+    file(GLOB engines "${CURRENT_PACKAGES_DIR}/lib/ossl-modules/*.dll")
+    if(engines)
+        file(COPY ${engines} DESTINATION "${CURRENT_PACKAGES_DIR}/bin")
+        file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/lib/ossl-modules")
+    endif()
+    file(GLOB engines "${CURRENT_PACKAGES_DIR}/debug/lib/ossl-modules/*.dll")
+    if(engines)
+        file(COPY ${engines} DESTINATION "${CURRENT_PACKAGES_DIR}/debug/bin")
+        file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/lib/ossl-modules")
+    endif()
+endif()


Does this need to be guarded by if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic" AND VCPKG_TARGET_IS_MINGW) since you are already checking if there are dll's in ossl-modules? If there are DLLs, they should be moved regardless IMO.

It seems robust enough to remove the extra guard now. OTOH, it make the purpose very clear.

JavierMatosD · 2023-02-23T22:51:14Z

@@ -1,31 +1,57 @@
-vcpkg_minimum_required(VERSION 2022-10-12) # for ${VERSION}


It looks like you're still using VERSION; should this be kept?

No. The check is obsolete since the check in scripts/ports.cmake was updated.

dg0yt

I can do the simlifications, but it seems in some spots it is remains necessay to say why it is this way.

dg0yt · 2023-02-24T04:37:33Z

@@ -1,31 +1,57 @@
-vcpkg_minimum_required(VERSION 2022-10-12) # for ${VERSION}


No. The check is obsolete since the check in scripts/ports.cmake was updated.

dg0yt · 2023-02-24T04:38:46Z

+file(MAKE_DIRECTORY "${CURRENT_PACKAGES_DIR}/tools/${PORT}")
+foreach(script IN ITEMS "bin/c_rehash")
+    file(COPY "${CURRENT_PACKAGES_DIR}/${script}" DESTINATION "${CURRENT_PACKAGES_DIR}/tools/${PORT}")
+    file(REMOVE "${CURRENT_PACKAGES_DIR}/${script}" "${CURRENT_PACKAGES_DIR}/debug/${script}")


It doesn't matter that much. We always have to remove the debug variant. So there will always be two commands.
The loop is obsolete now. OTOH it makes clear that c_rehash is a script. This fact is relevant with regard to vcpkg_copy_tools.

dg0yt · 2023-02-24T04:41:57Z

+    file(COPY "${CURRENT_PACKAGES_DIR}/${script}" DESTINATION "${CURRENT_PACKAGES_DIR}/tools/${PORT}")
+    file(REMOVE "${CURRENT_PACKAGES_DIR}/${script}" "${CURRENT_PACKAGES_DIR}/debug/${script}")
 endforeach()
+vcpkg_copy_tools(TOOL_NAMES openssl AUTO_CLEAN)


I cannot handle scripts with vcpkg_copy_tools because it only handles .exe for windows. And this code runs for native mingw builds.

dg0yt · 2023-02-24T04:44:05Z

+if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic" AND VCPKG_TARGET_IS_MINGW)
+    # For consistency with nmake build
+    file(GLOB engines "${CURRENT_PACKAGES_DIR}/lib/ossl-modules/*.dll")
+    if(engines)
+        file(COPY ${engines} DESTINATION "${CURRENT_PACKAGES_DIR}/bin")
+        file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/lib/ossl-modules")
+    endif()
+    file(GLOB engines "${CURRENT_PACKAGES_DIR}/debug/lib/ossl-modules/*.dll")
+    if(engines)
+        file(COPY ${engines} DESTINATION "${CURRENT_PACKAGES_DIR}/debug/bin")
+        file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/lib/ossl-modules")
+    endif()
+endif()


It seems robust enough to remove the extra guard now. OTOH, it make the purpose very clear.

dg0yt

@JavierMatosD Ready to review again.

dg0yt · 2023-02-26T18:33:07Z

+file(MAKE_DIRECTORY "${CURRENT_PACKAGES_DIR}/tools/${PORT}")
+foreach(script IN ITEMS "bin/c_rehash")
+    file(COPY "${CURRENT_PACKAGES_DIR}/${script}" DESTINATION "${CURRENT_PACKAGES_DIR}/tools/${PORT}")
+    file(REMOVE "${CURRENT_PACKAGES_DIR}/${script}" "${CURRENT_PACKAGES_DIR}/debug/${script}")


dg0yt · 2023-02-26T18:33:41Z

+if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic" AND VCPKG_TARGET_IS_MINGW)
+    # For consistency with nmake build
+    file(GLOB engines "${CURRENT_PACKAGES_DIR}/lib/ossl-modules/*.dll")
+    if(engines)
+        file(COPY ${engines} DESTINATION "${CURRENT_PACKAGES_DIR}/bin")
+        file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/lib/ossl-modules")
+    endif()
+    file(GLOB engines "${CURRENT_PACKAGES_DIR}/debug/lib/ossl-modules/*.dll")
+    if(engines)
+        file(COPY ${engines} DESTINATION "${CURRENT_PACKAGES_DIR}/debug/bin")
+        file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/lib/ossl-modules")
+    endif()
+endif()


…ndows (microsoft#27261)" This reverts commit 9ccc64c.

dg0yt force-pushed the openssl-unix branch 2 times, most recently from 9f57ced to 0ce019f Compare October 16, 2022 18:35

LilyWangLL assigned FrankXie05 Oct 17, 2022

FrankXie05 added the category:port-bug The issue is with a library, which is something the port should already support label Oct 17, 2022

dg0yt force-pushed the openssl-unix branch from be01c11 to 72c8861 Compare October 18, 2022 03:41

github-actions Bot requested changes Oct 18, 2022

View reviewed changes

dg0yt mentioned this pull request Oct 25, 2022

[scripts|nmake] Add jom option, add language control #27255

Merged

dg0yt added 3 commits January 17, 2023 08:11

Simplify non-windows openssl build

e2149af

No static libs in dynamic triplet

4492168

Install /etc/ssl

e9550ba

dg0yt force-pushed the openssl-unix branch from 72c8861 to 4ec1dcc Compare January 17, 2023 07:11

Refactor common and specific setup

59f60c7

dg0yt force-pushed the openssl-unix branch from 4ec1dcc to 59f60c7 Compare January 17, 2023 07:30

github-actions Bot previously approved these changes Jan 17, 2023

View reviewed changes

dg0yt changed the title ~~[openssl] Revise non-windows build~~ [openssl] Finish unified configuration for windows and non-windows Jan 18, 2023

dg0yt marked this pull request as ready for review January 18, 2023 07:17

dg0yt marked this pull request as draft January 18, 2023 17:23

This was referenced Jan 18, 2023

[aws-sdk-cpp] WIP #29045

Closed

[curl] Avoid targets in CURL_LIBRARIES #29061

Merged

dg0yt added 3 commits January 21, 2023 11:32

Fix debug configuration

d058d19

cleanup

c3c7aed

Remove deprecated no-ssl2 option

93fdb08

openssl/openssl@e80381e

dg0yt dismissed github-actions[bot]’s stale review via 93fdb08 January 21, 2023 11:01

github-actions Bot previously approved these changes Jan 21, 2023

View reviewed changes

dg0yt marked this pull request as ready for review January 21, 2023 11:08

dg0yt mentioned this pull request Feb 2, 2023

Difference in binary outputs in openssl package between Linux and Windows #29400

Closed

Merge remote-tracking branch 'origin/master' into openssl-unix

ce3a498

dg0yt dismissed github-actions[bot]’s stale review via ce3a498 February 10, 2023 22:06

github-actions Bot previously approved these changes Feb 10, 2023

View reviewed changes

Update linux kernel headers message

088fcad

dg0yt dismissed github-actions[bot]’s stale review via 088fcad February 16, 2023 08:01

github-actions Bot previously approved these changes Feb 16, 2023

View reviewed changes

LilyWangLL previously approved these changes Feb 17, 2023

View reviewed changes

FrankXie05 previously approved these changes Feb 17, 2023

View reviewed changes

FrankXie05 added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Feb 17, 2023

dg0yt mentioned this pull request Feb 21, 2023

Failed to install package with openssl dependency #27708

Closed

JavierMatosD suggested changes Feb 23, 2023

View reviewed changes

JavierMatosD added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Feb 23, 2023

dg0yt commented Feb 24, 2023

View reviewed changes

CR requests

b5de6e5

dg0yt dismissed stale reviews from FrankXie05, LilyWangLL, and github-actions[bot] via b5de6e5 February 26, 2023 18:32

dg0yt commented Feb 26, 2023

View reviewed changes

JavierMatosD approved these changes Feb 28, 2023

View reviewed changes

JavierMatosD merged commit 9ccc64c into microsoft:master Feb 28, 2023

dg0yt deleted the openssl-unix branch February 28, 2023 21:00

AenBleidd mentioned this pull request Mar 1, 2023

[openssl] build failure on Android #29947

Closed

This was referenced Mar 3, 2023

[openssl] Build error with arm64-linux triplet #29674

Closed

[openssl] i use custom triplets, but gcc have wrong path #27961

Closed

[openssl] Build error #26634

Closed

This was referenced Mar 5, 2023

[openssl,vcpkg-cmake-get-vars] Fix android and emscripten, add feature tools #29966

Merged

[openssl] build failure on wasm32-emscripten #30037

Closed

talregev added a commit to talregev/vcpkg that referenced this pull request Mar 6, 2023

Revert "[openssl] Finish unified configuration for windows and non-wi…

644daea

…ndows (microsoft#27261)" This reverts commit 9ccc64c.

talregev mentioned this pull request Mar 7, 2023

[openssl] Revert. support for android and wasm32-emscripten #30053

Closed

		@@ -1,31 +1,57 @@
		vcpkg_minimum_required(VERSION 2022-10-12) # for ${VERSION}

Uh oh!

Conversation

dg0yt commented Oct 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

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

Tests

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

dg0yt commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dg0yt commented Feb 16, 2023

Uh oh!

JavierMatosD left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dg0yt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dg0yt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dg0yt commented Oct 16, 2022 •

edited

Loading

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

dg0yt commented Jan 18, 2023 •

edited

Loading