[vcpkg-cmake] Update parallel vcpkg_cmake_configure by dg0yt · Pull Request #21507 · microsoft/vcpkg

dg0yt · 2021-11-18T06:57:46Z

What does your PR fix?

Enables parallel cmake whenever ninja is available (non-windows hosts, system Ninja).
Adds "Unix Makefiles" as fallback generator for non-Windows system.
Establishs this order for generator selection:
- If the host is x86 or VCPKG_FORCE_SYSTEM_BINARIES is set, try to find Ninja via find_program.
  - It it cannot be found, then assume that msbuild is requested for windows.
- If msbuild is requested and suitable, use it.
- Else if a particular generator is requested, use it.
- Else if ninja is available, use it.
- Else on non-windows host, use "Unix Makefiles".
Which triplets are supported/not supported? Have you updated the CI baseline?

all, 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?

incomplete

JackBoosY · 2021-11-18T07:15:40Z

Should we also fix vcpkg_configure_cmake?

dg0yt · 2021-11-18T07:21:50Z

Should we also fix vcpkg_configure_cmake?

I tend to not change it:

Ports shall be switched to vcpkg_cmake_configure for corrected behaviour.
No behavioural change for building old versions of ports when they use vcpkg_configure_cmake.

Neumann-A · 2021-11-18T07:39:01Z

i would really like separate logs for each config if it is possible.

dg0yt · 2021-11-18T07:42:04Z

i would really like separate logs for each config if it is possible.

Understood. However, this wasn't the case for windows before.
Note that within the single log, ninja clearly separates the individual outputs. (Tested locally with port cpr.)

Neumann-A · 2021-11-18T07:52:52Z

Understood. However, this wasn't the case for windows before.
Note that within the single log, ninja clearly separates the individual outputs. (Tested locally with port cpr.)

Yeah I know. Windows always puts everything in the same file which is kind of annoying and harder to debug in my view.
The main purpose is

make the files comparable to easily see differences in configure between dbg/rel
make the errors more visible due to the separate logs

dg0yt · 2021-11-18T08:38:05Z

i would really like separate logs for each config if it is possible.

I don't see a simple way to do that portably in the ninja rule, for stdout and stderr.
This leaves only some post processing which also isn't trivial after a failed job.

OTOH the single config log does have some advantages:

The number of files is reduced.
Release config output is available even when debug config failed.

dg0yt · 2021-11-18T15:08:42Z

I don't see a simple way to do that portably in the ninja rule, for stdout and stderr.

cmd has the same redirection syntax as sh. cmd /C for windows then?
If yes, I would also move the cmake chdir to a plain cd.

dg0yt · 2021-11-18T19:23:30Z

bcg729:x64-linux: source write, fix in #21516.

dg0yt · 2021-11-19T06:24:32Z

i would really like separate logs for each config if it is possible.

I'm also curious how you would think about parallelizing the build step. If this is possible at all, via subninja, there is no way to redirect the output. (However, I would leave the install step serialized.)

dg0yt · 2021-11-20T11:11:29Z

I don't see a simple way to do that portably in the ninja rule, for stdout and stderr.

cmd has the same redirection syntax as sh. cmd /C for windows then? If yes, I would also move the cmake chdir to a plain cd.

The cmd approach is doable, writing the command to a cmd file instead of build.ninja. Now the problem is a different one:
vcpkg_execute_required_process is unaware of the log files created via redirection. In case of error, it won't tell you to look at the split files.

dg0yt · 2021-11-23T05:38:34Z

This PR is twofold, a) fixing the working directory for parallel configure, and b) applying parallel configure outside windows. Since it rebuilds a lot of ports, it should be decided if b) shall be left in this PR.
IMO the change is okay, because the behaviour is unified, ninja already takes care of output order, and I prefer to see output of both build types on errors.

JackBoosY · 2021-11-24T09:22:59Z

Waiting for test results.

dg0yt · 2021-11-24T09:41:15Z

This is now expensively rebuilding against an old version of master. Better merge HEAD of master, to fetch the bcg729 update.
(I didn't want to trigger CI by a PR update before knowing the desired direction.)

JackBoosY · 2021-11-24T09:44:11Z

@dg0yt Fine, so please merge to master to retrigger the ci test.

JackBoosY · 2021-11-25T08:37:39Z

hpx regression was fixed in #21617.
python3:x64-osx:

install: mkdir /Users/vagrant/Data/packages/python3_x64-osx/Users/vagrant/Data/installed/x64-osx/debug/lib: File exists
make: *** [altbininstall] Error 71

wavelib:arm-uwp:

       CUSTOMBUILD : CMake error : Cannot restore timestamp "D:/buildtrees/wavelib/arm-uwp-rel/test/CMakeFiles/generate.stamp": Access is denied. [D:\buildtrees\wavelib\arm-uwp-rel\test\modwttest.vcxproj]

dg0yt · 2021-11-26T06:58:01Z

wavelib has parallel source writes for test executables, #21671.

dg0yt · 2021-11-26T12:06:48Z

python3:x64-osx:

install: mkdir /Users/vagrant/Data/packages/python3_x64-osx/Users/vagrant/Data/installed/x64-osx/debug/lib: File exists
make: *** [altbininstall] Error 71

In install-x64-osx-dbg-out.log:

...
Creating directory /Users/vagrant/Data/installed/x64-osx/debug/lib
Creating directory /Users/vagrant/Data/installed/x64-osx/debug/lib
...

This error is not related to this PR: python3 uses cmake only for windows incl. uwp. The osx build uses configure+make.
make altinstall has a race condition: In Makefile.def, there are two subtargets which create $(DESTDIR)$(LIBDIR) after checking for its existence. This must be serialized.

If the host is x86, assume that msbuild is requested for windows. If msbuild is requested and suitable, use it. Else if a particular generator is requested, use it. Else if ninja is available, use it. Else on non-windows host, use "Unix Makefiles".

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

Diff

diff --git a/versions/baseline.json b/versions/baseline.json
index 471b840..5d0b378 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -4741,7 +4741,7 @@
       "port-version": 1
     },
     "ms-gltf": {
-      "baseline": "2022-06-28",
+      "baseline": "2022-07-18",
       "port-version": 0
     },
     "ms-gsl": {
@@ -7421,7 +7421,7 @@
       "port-version": 0
     },
     "vcpkg-cmake": {
-      "baseline": "2022-07-02",
+      "baseline": "2022-07-18",
       "port-version": 0
     },
     "vcpkg-cmake-config": {
diff --git a/versions/m-/ms-gltf.json b/versions/m-/ms-gltf.json
index 87af4d8..69b11e9 100644
--- a/versions/m-/ms-gltf.json
+++ b/versions/m-/ms-gltf.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "1573e63635b8505134e0ecddcb2f0ab76874fb54",
+      "version-date": "2022-07-18",
+      "port-version": 0
+    },
     {
       "git-tree": "a9a91635168ea77faa39adb73b27483797fa8967",
       "version-date": "2022-06-28",
diff --git a/versions/v-/vcpkg-cmake.json b/versions/v-/vcpkg-cmake.json
index daad9b9..a329f1d 100644
--- a/versions/v-/vcpkg-cmake.json
+++ b/versions/v-/vcpkg-cmake.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "a7b618b7782f3c841d7fd2d84a6ba3619815362a",
+      "version-date": "2022-07-18",
+      "port-version": 0
+    },
     {
       "git-tree": "94abbd71a7fe495e883b13c077312f6d419cbc41",
       "version-date": "2022-07-02",

dg0yt · 2022-07-18T06:27:27Z

I will update the version database when the PR received preliminary acceptance.

cenit · 2022-07-19T15:44:52Z

ports/ms-gltf/vcpkg.json

should bump port version, not version-date

That's what I would have done for other version schemes.

If the content of the stuff we install changed, then the version-date should be updated. If only maintainer improvement stuff in the port changed, port-version should be bumped.

dg0yt · 2022-07-20T05:39:42Z

It is nice to know that there is a micro issue with the ms-gltf version update. But before burning more CI time, I need feedback on the core part.

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

Diff

diff --git a/versions/baseline.json b/versions/baseline.json
index 0114230..89c2864 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -4742,7 +4742,7 @@
     },
     "ms-gltf": {
       "baseline": "2022-06-28",
-      "port-version": 0
+      "port-version": 1
     },
     "ms-gsl": {
       "baseline": "4.0.0",
@@ -7421,7 +7421,7 @@
       "port-version": 0
     },
     "vcpkg-cmake": {
-      "baseline": "2022-07-02",
+      "baseline": "2022-07-18",
       "port-version": 0
     },
     "vcpkg-cmake-config": {
diff --git a/versions/m-/ms-gltf.json b/versions/m-/ms-gltf.json
index 87af4d8..82b9a8b 100644
--- a/versions/m-/ms-gltf.json
+++ b/versions/m-/ms-gltf.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "b189e4d23ebe85437573b386d94b06b3f9fb6238",
+      "version-date": "2022-06-28",
+      "port-version": 1
+    },
     {
       "git-tree": "a9a91635168ea77faa39adb73b27483797fa8967",
       "version-date": "2022-06-28",
diff --git a/versions/v-/vcpkg-cmake.json b/versions/v-/vcpkg-cmake.json
index daad9b9..a329f1d 100644
--- a/versions/v-/vcpkg-cmake.json
+++ b/versions/v-/vcpkg-cmake.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "a7b618b7782f3c841d7fd2d84a6ba3619815362a",
+      "version-date": "2022-07-18",
+      "port-version": 0
+    },
     {
       "git-tree": "94abbd71a7fe495e883b13c077312f6d419cbc41",
       "version-date": "2022-07-02",

BillyONeal

I note that there is a preexisting issue related to vcpkg_find_acquire_program and FORCE_SYSTEM_BINARIES I did not comment on here since it's unrelated to this improvement. (And I don't think other reviewers should block for that either)

BillyONeal · 2022-07-25T19:23:14Z

Thanks for making parallelism work in more places:)

dg0yt · 2022-07-25T19:36:55Z

Now I may turn to vpckg_configure_make. Which has quite some potential.

Neumann-A · 2022-07-25T19:39:54Z

Now I may turn to vpckg_configure_make. Which has quite some potential.

do/test it outside of that function ;) Make a script port for it.

github-actions bot approved these changes Nov 18, 2021

View reviewed changes

JonLiu1993 assigned JonLiu1993 and JackBoosY and unassigned JonLiu1993 Nov 18, 2021

JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Nov 18, 2021

dg0yt mentioned this pull request Nov 18, 2021

[bcg729] Fix cmake, fix parallel build on linux #21516

Merged

JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label Nov 19, 2021

JackBoosY removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Nov 23, 2021

github-actions bot approved these changes Nov 24, 2021

View reviewed changes

dg0yt mentioned this pull request Nov 26, 2021

[vcpkg baseline][wavelib] Fix parallel build #21671

Merged

dg0yt mentioned this pull request Nov 28, 2021

[vcpkg baseline][python3] Fix race during parallel install #21734

Merged

github-actions bot approved these changes Nov 29, 2021

View reviewed changes

dg0yt added 6 commits July 18, 2022 08:14

Revise generator selection

105c041

If the host is x86, assume that msbuild is requested for windows. If msbuild is requested and suitable, use it. Else if a particular generator is requested, use it. Else if ninja is available, use it. Else on non-windows host, use "Unix Makefiles".

Revise ninja_host detection

63d404e

Revise parallel configure detection

6c5a433

Consolidate ninja path setup

2fdbf10

Update documentation

de9c3a4

Use portable chdir

1d63c0b

dg0yt force-pushed the parallel-cmake branch from 6ee0b4c to 934505f Compare July 18, 2022 06:26

github-actions bot requested changes Jul 18, 2022

View reviewed changes

github-actions bot previously approved these changes Jul 19, 2022

View reviewed changes

dg0yt dismissed github-actions[bot]’s stale review via afcf581 July 19, 2022 06:34

dg0yt force-pushed the parallel-cmake branch from 5a2bcc0 to afcf581 Compare July 19, 2022 06:34

cenit reviewed Jul 19, 2022

View reviewed changes

dg0yt marked this pull request as ready for review July 20, 2022 05:38

[ms-gltf] Use new generator selection

d154b1f

dg0yt force-pushed the parallel-cmake branch from afcf581 to d154b1f Compare July 20, 2022 05:49

Update versions

ffbbca6

github-actions bot requested changes Jul 20, 2022

View reviewed changes

github-actions bot approved these changes Jul 20, 2022

View reviewed changes

BillyONeal approved these changes Jul 20, 2022

View reviewed changes

JackBoosY approved these changes Jul 21, 2022

View reviewed changes

JackBoosY added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jul 21, 2022

BillyONeal merged commit 49868fd into microsoft:master Jul 25, 2022

dg0yt deleted the parallel-cmake branch July 25, 2022 19:36

Conversation

dg0yt commented Nov 18, 2021 • 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?

Uh oh!

JackBoosY commented Nov 18, 2021

Uh oh!

dg0yt commented Nov 18, 2021

Uh oh!

Neumann-A commented Nov 18, 2021

Uh oh!

dg0yt commented Nov 18, 2021

Uh oh!

Neumann-A commented Nov 18, 2021

Uh oh!

dg0yt commented Nov 18, 2021

Uh oh!

dg0yt commented Nov 18, 2021

Uh oh!

dg0yt commented Nov 18, 2021

Uh oh!

dg0yt commented Nov 19, 2021

Uh oh!

dg0yt commented Nov 20, 2021

Uh oh!

dg0yt commented Nov 23, 2021

Uh oh!

JackBoosY commented Nov 24, 2021

Uh oh!

dg0yt commented Nov 24, 2021

Uh oh!

JackBoosY commented Nov 24, 2021

Uh oh!

JackBoosY commented Nov 25, 2021

Uh oh!

dg0yt commented Nov 26, 2021

Uh oh!

dg0yt commented Nov 26, 2021

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

dg0yt commented Jul 18, 2022

Uh oh!

cenit Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

dg0yt Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

BillyONeal Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

dg0yt commented Jul 20, 2022

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

BillyONeal left a comment

Choose a reason for hiding this comment

Uh oh!

BillyONeal commented Jul 25, 2022

Uh oh!

dg0yt commented Jul 25, 2022

Uh oh!

Neumann-A commented Jul 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dg0yt commented Nov 18, 2021 •

edited

Loading

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