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

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

Merged
merged 10 commits into from
Nov 2, 2022

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Oct 16, 2022

Pulled out of #27150:

  • What does your PR fix?

    Two independent contributions:

    1. Allow proper _CL_ setup for C projects, fixing (re-enabling) uwp builds for nmake C projects.
    2. Add opt-in concurrency support via jom (inspired by and targeting port openssl, but also beneficial for libspatialite etc.)
  • Which triplets are supported/not supported? Have you updated the CI baseline?

    windows, yes

  • 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 Oct 16, 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/librttopo/vcpkg.json
  • ports/spatialite-tools/vcpkg.json

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

[PROJECT_SUBPATH <${SUBPATH}>]
[PROJECT_NAME <${MAKEFILE_NAME}>]
[LOGFILE_ROOT <prefix>]
[CL_LANGUAGE <language-name>]
[PREFER_JOM]
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably no. Have jom be the default and deactivate it with a VCPKG_NMAKE_NO_JOM ?. similar how qmake has VCPKG_QMAKE_USE_NMAKE. The reason not to even try using jom is that Jom has consistencies issues on some machines and keeps dead locking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"No" what?
jom is proposed as opt-in just because it has the potential to break working builds.
(For openssl, the most important mitigation is to configure with no-makedepend.)

Copy link
Contributor Author

@dg0yt dg0yt Oct 16, 2022

Choose a reason for hiding this comment

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

Qt came with its own tool for generating (n)makefiles. In the Qt world, jom compatibility is a reasonable assumption. But outside Qt, we can't rely on makefiles being designed and tested for concurrency.

@Cheney-W Cheney-W added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly category:documentation To resolve the issue, documentation will need to be updated 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 labels Oct 17, 2022
@vicroms vicroms added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed info:reviewed Pull Request changes follow basic guidelines labels Oct 17, 2022
@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 25, 2022

@vicroms et al. Please don't let this wait too long in requires:vcpkg-team-review. I would like to continue the openssl cleanup (#27150, #27261).

  • The basic pattern jom -j <N> -k || nmake is not new but was used for a while in port openssl.
    (I changed it to jom -j <N> -k || jom -j 1 some months ago but it turns out that jom has problems at least with some version of ConEmu.)

  • The PREFER_JOM argument is chosen in analogy to the PREFER_NINJA argument of the (deprecated) vcpkg_configure_cmake.

  • Data points for the adapted ports, x64-windows CI:

    Port nmake build jom build
    libspatialite 1.1 min 1.1 min
    openssl 16 min (768819cf4bf6752c30c0e44d7b4dfad91b822f9c) 1.7 min (master)
    readosm 3.5 s ?
    spatialite-tools 21 s 22 s

    So I acknowledge that the jom option only helps openssl at the moment.
    Reverting the PREFER_JOM to the other ports should pass CI quickly.

  • There are ports which use nmake but do not the maintainer functions:

    • isal (uses vcpkg_build_nmake)
    • libtomcrypt
    • libtommath
    • pthreads
    • sqlcipher (use nmake to create an amalgamation, then builds with cmake)

    They should benefit from the uniform toolchain setup. But nmake build times are not nearly as bad as openssl.
    This is for separate PRs.

  • There may be other ports which use a vendored CMakeLists.txt instead of upstream's nmake Makefile.
    If the native build benefits from jom, this may remove one reason for the existence of the vendored build system.
    This is for separate PRs.

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 31, 2022

@vicroms Ping again.

@BillyONeal BillyONeal added the info:reviewed Pull Request changes follow basic guidelines label Nov 1, 2022
@BillyONeal
Copy link
Member

Tagging reviewed so that I look at this tomorrow.

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.

This looks correct to me, in particular the dispute over PREFER_JOM here preserving backwards compatibility seems to be resolved correctly to me.

I'm going to poke @vicroms in person one more time and merge this if I get no reply tomorrow.

@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Nov 2, 2022
@BillyONeal BillyONeal merged commit 3e35cb0 into microsoft:master Nov 2, 2022
@BillyONeal
Copy link
Member

Thanks for the help @dg0yt !

@dg0yt dg0yt deleted the nmake-jom-c branch November 2, 2022 18:32
@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 2, 2022

Now I can look into openssl again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:documentation To resolve the issue, documentation will need to be updated category:port-bug The issue is with a library, which is something the port should already support category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants