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

[openssl] Revise jom/nmake support #27150

Merged
merged 4 commits into from
Nov 11, 2022
Merged

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Oct 9, 2022

  • What does your PR fix?

    Uniformly use plain vcpkg_build_nmake for windows incl. nmake, leveraging the recently integrated PREFER_JOM with nmake fallback. This fallback should mitigate issues with running jom on some computers.
    It also cherry picks a post-3.0.7 fixup from [openssl] Generate OPENSSL_VERSION_MAJOR/MINOR/FIX #27618.

    PR history:
    Based on various issues reported for building port openssl on windows, this PR started as an attempt to build openssl without jom using plain vcpkg_build_nmake but turned into a revision of this script to integrate parallel build capabilities using jom. These script changes were merged as [scripts|nmake] Add jom option, add language control #27255.
    Some past pushes also tested de-duplicating steps shared by the unix and windows branches.
    But now this PR is trimmed.

  • 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

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 5a9f73ae88cfca3f1a13a75c0a622a028a5bfa0b -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 82dd51f..1a26e53 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -5434,7 +5434,7 @@
     },
     "openssl": {
       "baseline": "3.0.5",
-      "port-version": 4
+      "port-version": 5
     },
     "openssl-unix": {
       "baseline": "1.1.1h",
diff --git a/versions/o-/openssl.json b/versions/o-/openssl.json
index bbcf02a..8102b7e 100644
--- a/versions/o-/openssl.json
+++ b/versions/o-/openssl.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "7b32a391596b6db7554f75f57cda0ecf61282fcc",
+      "version": "3.0.5",
+      "port-version": 5
+    },
     {
       "git-tree": "557ff31f9a64f01cd0d98dd44793ce3c7fd32893",
       "version": "3.0.5",

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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for openssl have changed but the version was not updated
version: 3.0.5#5
old SHA: 067a790dfd1559e77d5a199ccbe982322882d667
new SHA: ba84f7992d88c81eb785e1c4f1ffdaf70e791048
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@Cheney-W Cheney-W added the category:port-bug The issue is with a library, which is something the port should already support label Oct 9, 2022
@dg0yt dg0yt force-pushed the openssl-nmake branch 3 times, most recently from 768819c to e0098cc Compare October 11, 2022 06:23
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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for openssl have changed but the version was not updated
version: 3.0.5#5
old SHA: 067a790dfd1559e77d5a199ccbe982322882d667
new SHA: a64737cddb0d2dfd8b52478c8ce1385b9450dc51
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 11, 2022

We probably won't remove jom due to build time with nmake, but there is still enough to take from this PR:

  • General port cleanup
  • Merge uwp rules into windows rules

There is still room for improvement, in particular where I don't know enough about the MSVC toolchain:

  • More clear resolve the PDF/-Zi/-Z7 handling?
  • Filter the -MP from concurrent builds (' jom')?

Another option is including jom as an option into vcpkg_build_nmake.

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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for openssl have changed but the version was not updated
version: 3.0.5#5
old SHA: 067a790dfd1559e77d5a199ccbe982322882d667
new SHA: b5a37fdf492be2f95951b19fbb344dc3683d2f15
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@dg0yt dg0yt changed the title [openssl] Build without jom [openssl] Revise jom/nmake support Oct 13, 2022
@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 14, 2022

Hm, I think I pass the same options to jom and nmake, but CC (full path) arrives only in nmake log files, not jom makefiles.
OTOH, I wasn't able to create a minimal reproducer with jom and echo $(CC) 😠

@dg0yt dg0yt force-pushed the openssl-nmake branch 3 times, most recently from 9086023 to 22c66a1 Compare October 15, 2022 04:04
@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 16, 2022

The current opencv4 regression is expected to be fixed by #27239.

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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for openssl have changed but the version was not updated
version: 3.0.5#5
old SHA: 067a790dfd1559e77d5a199ccbe982322882d667
new SHA: 43406963dd80e11d9d797ef3adb3e171e8863994
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 19, 2022

Depends on #27255.

@Cheney-W Cheney-W added the depends:different-pr This PR or Issue depends on a PR which has been filed label Oct 28, 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.

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

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for openssl have changed but the version was not updated
version: 3.0.7
old SHA: 09701bf7506bd0d161bf671eff1c7f5b3d73e3a9
new SHA: a538703a7f007bfbd879e0dbde047bcb3105467d
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@Cheney-W Cheney-W removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Nov 3, 2022
@dg0yt dg0yt force-pushed the openssl-nmake branch 2 times, most recently from a3f6e88 to 1b5a366 Compare November 4, 2022 19:02
github-actions[bot]
github-actions bot previously approved these changes Nov 4, 2022
github-actions[bot]
github-actions bot previously approved these changes Nov 4, 2022
@dg0yt dg0yt marked this pull request as ready for review November 9, 2022 07:01
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 am far from an expert in this area but everything here seems reasonable. I've also closed my PR that this cherry picks.

@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Nov 11, 2022
@JavierMatosD
Copy link
Contributor

JavierMatosD commented Nov 11, 2022

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants