[vcpkg_execute_*_process] Add support for build profiling #22676

mkhon · 2022-01-21T07:26:38Z

Describe the pull request

Use --x-cmake-args=-DX_PORT_PROFILE=TRUE to enable profiling

What does your PR fix?
Which triplets are supported/not supported? Have you updated the CI baseline?

all

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

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

Neumann-A · 2022-01-21T12:06:05Z

i don't see why this should be an env variable. You cann simply pass --x-cmake-args=<value> to vcpkg (this deactivates binary caching but the env approach suggest also changes the hash of binary caching which is undesired unless the variable is added to UNTRACKED )

mkhon · 2022-01-21T20:07:13Z

i don't see why this should be an env variable. You cann simply pass --x-cmake-args=<value> to vcpkg (this deactivates binary caching but the env approach suggest also changes the hash of binary caching which is undesired unless the variable is added to UNTRACKED )

I updated the patch so that it now uses a plain variable

dg0yt · 2022-01-22T18:30:29Z

scripts/cmake/vcpkg_acquire_msys.cmake

+    z_vcpkg_acquire_msys_declare_package(
+        URL "https://repo.msys2.org/msys/x86_64/time-1.9-2-x86_64.pkg.tar.zst"
+        SHA512 923607127f3a9c644136a3e6cc76885ddcd75e7f964e4732d6897af72ca334af5c8bd69debde19985c8e785979006da41d0df34a230b6bdebd5b6ca2ab29774b
+    )


Is this really needed? There is cmake -E time <cmd>.

vcpkg also logs elapsed time required to build a package.
This change allows to measure configure/build stages separately.

I used it to measure libtool-related optimization fix (cuts off about 2x from gettext[tools] build time) and just submitted it because I thought it might be generally useful.

I know what you are trying to achieve. I'm proposing you shouldn't use the time command which doesn't exist on Windows but the portable alternative offered by CMake.

dg0yt · 2022-01-22T18:34:29Z

scripts/cmake/vcpkg_execute_build_process.cmake

+        list(GET arg_COMMAND 0 command)
+        if(NOT command MATCHES [[/msys2/]])
+            message(STATUS "Using MSYS2_ARG_CONV_EXCL=*")
+            set(ENV{MSYS2_ARG_CONV_EXCL} "*")


This changes msys2 behaviour for all commands. This may create unexpected problems. Same for the other scripts where this pattern is used.

May be I am missing something, but on the contrary - this chunk is to not change anything for msys2 and non-msys2 (do not convert arguments) commands

This environment variable affects conversions deeper down below the initial command:

With PORT_PROFILE false: Default or user-defined msys argument conversion behaviour.

With PORT_PROFILE true: No msys argument conversion at all.

We don't know how often the flow of control crosses between msys (e.g. make) and windows (e.g. cmake) environments. If it works now (no PORT_PROFILE), it can break with PORT_PROFILE evaluating to true.
So it is a ticking bomb IMO.

@dg0yt I re-implemented the change using "cmake -E time"

JackBoosY · 2022-01-25T06:27:10Z

@BillyONeal Any suggestions here?

ras0219-msft

This LGTM, except that we'd like to first introduce this as an experimental flag (X_....).

I would also suggest reversing the flag name to be X_PROFILE_PORT, but that's a looser suggestion.

Thanks!

mkhon · 2022-01-29T06:37:22Z

This LGTM, except that we'd like to first introduce this as an experimental flag (X_....).

I would also suggest reversing the flag name to be X_PROFILE_PORT, but that's a looser suggestion.

Thanks!

I was just following the "convention" of PORT_DEBUG flag

mkhon · 2022-01-29T06:44:24Z

This LGTM, except that we'd like to first introduce this as an experimental flag (X_....).

I would also suggest reversing the flag name to be X_PROFILE_PORT, but that's a looser suggestion.

Thanks!

I updated the PR to use X_PORT_PROFILE

JackBoosY · 2022-02-06T03:11:40Z

FAILED: test/integration/INTEGRATION_locale_fix.exe 
cmd.exe /C "cd . && D:\downloads\tools\cmake-3.21.1-windows\cmake-3.21.1-windows-i386\bin\cmake.exe -E vs_link_exe --intdir=test\integration\CMakeFiles\INTEGRATION_locale_fix.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100190~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100190~1.0\x64\mt.exe --manifests  -- C:\PROGRA~2\MICROS~3\2019\ENTERP~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\link.exe  test\integration\CMakeFiles\INTEGRATION_locale_fix.dir\locale_fix.cc.obj test\integration\CMakeFiles\INTEGRATION_locale_fix.dir\__\__\src\win\tinyxml\tinystr.cpp.obj test\integration\CMakeFiles\INTEGRATION_locale_fix.dir\__\__\src\win\tinyxml\tinyxmlerror.cpp.obj test\integration\CMakeFiles\INTEGRATION_locale_fix.dir\__\__\src\win\tinyxml\tinyxml.cpp.obj test\integration\CMakeFiles\INTEGRATION_locale_fix.dir\__\__\src\win\tinyxml\tinyxmlparser.cpp.obj  /out:test\integration\INTEGRATION_locale_fix.exe /implib:test\integration\INTEGRATION_locale_fix.lib /pdb:test\integration\INTEGRATION_locale_fix.pdb /version:0.0 /nologo /DEBUG /INCREMENTAL:NO /OPT:REF /OPT:ICF   /subsystem:console -LIBPATH:D:\buildtrees\sdformat6\x64-windows-rel\src   -LIBPATH:D:\buildtrees\sdformat6\x64-windows-rel\test gtest.lib  gtest_main.lib  sdformat.lib  D:\installed\x64-windows\lib\ignition-math4.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && cmd.exe /C "cd /D D:\buildtrees\sdformat6\x64-windows-rel\test\integration && D:\downloads\tools\cmake-3.21.1-windows\cmake-3.21.1-windows-i386\bin\cmake.exe -E copy_if_different D:/buildtrees/sdformat6/x64-windows-rel/src/sdformat.dll D:/buildtrees/sdformat6/x64-windows-rel/test/integration""
Error copying file (if different) from "D:/buildtrees/sdformat6/x64-windows-rel/src/sdformat.dll" to "D:/buildtrees/sdformat6/x64-windows-rel/test/integration".

Not related to this changes.

JackBoosY · 2022-02-21T09:41:08Z

Ping @ras0219-msft for review this again.

JackBoosY · 2022-03-18T08:17:56Z

Can you please merge this branch to master so I can review again?

Thanks!

JackBoosY · 2022-05-05T06:39:52Z

Depends on #24542.

BillyONeal · 2022-05-06T04:47:18Z

I killed the job attempting to "recheck" here because it wouldn't have gotten the merge of the dependent PR. We need a merge with master to pick up #24542 now that it has been merged.

JackBoosY · 2022-05-06T05:58:57Z

Please merge this branch to master, we don't have the permission to push any changes.

mkhon · 2022-05-07T06:55:50Z

Please merge this branch to master, we don't have the permission to push any changes.

Done

JackBoosY · 2022-05-09T03:03:59Z

When building mesa[core,default-features,gles1,gles2,llvm,opengl]:x64-windows:

../src/esa-21.2.5-2df234d2b1.clean/src/compiler/glsl/glcpp/glcpp-lex.l(624): error C2059: syntax error: 'constant'
../src/esa-21.2.5-2df234d2b1.clean/src/compiler/glsl/glcpp/glcpp-lex.l(624): error C2059: syntax error: 'bad suffix on number'
../src/esa-21.2.5-2df234d2b1.clean/src/compiler/glsl/glcpp/glcpp-lex.l(624): error C2001: newline in constant
../src/esa-21.2.5-2df234d2b1.clean/src/compiler/glsl/glcpp/glcpp-lex.l(647): error C2059: syntax error: 'break'
../src/esa-21.2.5-2df234d2b1.clean/src/compiler/glsl/glcpp/glcpp-lex.l(647): error C2059: syntax error: ']'
../src/esa-21.2.5-2df234d2b1.clean/src/compiler/glsl/glcpp/glcpp-lex.l(649): error C2059: syntax error: 'while'
../src/esa-21.2.5-2df234d2b1.clean/src/compiler/glsl/glsl_lexer.ll(339): error C2059: syntax error: '['
../src/esa-21.2.5-2df234d2b1.clean/src/compiler/glsl/glsl_lexer.ll(343): error C2059: syntax error: 'break'

But file glcpp-lex.l only have 622 lines.
@Neumann-A @cenit Any ideas?

JackBoosY · 2022-05-13T09:52:57Z

@BillyONeal I think we can review this again.

BillyONeal · 2022-05-16T21:26:10Z

Is there a reason we shouldn't just always do this instead of requiring use of a flag?

JackBoosY · 2022-05-18T09:37:30Z

Ping @mkhon

mkhon · 2022-05-31T14:18:59Z

I removed X_PORT_PROFILE in the new version

mkhon · 2022-06-02T13:14:17Z

The failures I see are caused by individual port build errors and are not related to the change I suggest

JackBoosY · 2022-06-06T08:57:13Z

Can you please merge this branch to master since I have no permission to push to this branch?

Use --x-cmake-args=-DX_PORT_PROFILE=TRUE to enable port build profiling

mkhon · 2022-06-06T12:40:47Z

Can you please merge this branch to master since I have no permission to push to this branch?

Done

JackBoosY · 2022-06-08T09:53:08Z

air-ctl regression is fixing in #25113.

ras0219-msft · 2022-06-10T17:57:07Z

I think we might consider turning this on be default in the future, but this is at least a nice feature to have experimentally. Thanks for your contribution!

dg0yt · 2022-06-10T18:58:30Z

Thanks for this PR!

github-actions bot approved these changes Jan 21, 2022

View reviewed changes

JackBoosY self-assigned this Jan 21, 2022

JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Jan 21, 2022

JackBoosY requested a review from BillyONeal January 21, 2022 07:50

github-actions bot approved these changes Jan 21, 2022

View reviewed changes

github-actions bot approved these changes Jan 22, 2022

View reviewed changes

dg0yt reviewed Jan 22, 2022

View reviewed changes

github-actions bot approved these changes Jan 24, 2022

View reviewed changes

JackBoosY added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 27, 2022

ras0219-msft requested changes Jan 28, 2022

View reviewed changes

github-actions bot approved these changes Jan 29, 2022

View reviewed changes

github-actions bot approved these changes Jan 30, 2022

View reviewed changes

github-actions bot approved these changes Feb 4, 2022

View reviewed changes

JackBoosY changed the title ~~Add support for build profiling~~ [vcpkg_execute_*_process]Add support for build profiling Feb 6, 2022

JackBoosY changed the title ~~[vcpkg_execute_*_process]Add support for build profiling~~ [vcpkg_execute_*_process] Add support for build profiling Feb 6, 2022

JackBoosY added depends:different-pr This PR or Issue depends on a PR which has been filed and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Feb 6, 2022

JackBoosY mentioned this pull request Feb 7, 2022

[sdformat6] Disable test #22970

Merged

JackBoosY removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Feb 8, 2022

JackBoosY requested a review from ras0219-msft February 21, 2022 09:41

JackBoosY approved these changes Mar 18, 2022

View reviewed changes

JackBoosY added depends:different-pr This PR or Issue depends on a PR which has been filed and removed requires:author-response depends:different-pr This PR or Issue depends on a PR which has been filed labels May 5, 2022

JackBoosY added the requires:author-response label May 6, 2022

github-actions bot approved these changes May 7, 2022

View reviewed changes

JackBoosY removed the requires:author-response label May 7, 2022

JackBoosY added the requires:author-response label May 18, 2022

github-actions bot approved these changes May 31, 2022

View reviewed changes

Add support for build profiling

2962f78

Use --x-cmake-args=-DX_PORT_PROFILE=TRUE to enable port build profiling

github-actions bot approved these changes Jun 6, 2022

View reviewed changes

JackBoosY removed the requires:author-response label Jun 10, 2022

JackBoosY approved these changes Jun 10, 2022

View reviewed changes

JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jun 10, 2022

ras0219-msft merged commit 2e4c6df into microsoft:master Jun 10, 2022

mkhon deleted the feature/build-profiling branch June 10, 2022 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vcpkg_execute_*_process] Add support for build profiling #22676

[vcpkg_execute_*_process] Add support for build profiling #22676

mkhon commented Jan 21, 2022 •

edited

Loading

Neumann-A commented Jan 21, 2022

mkhon commented Jan 21, 2022

dg0yt Jan 22, 2022

mkhon Jan 23, 2022

dg0yt Jan 23, 2022 •

edited

Loading

dg0yt Jan 22, 2022

mkhon Jan 23, 2022

dg0yt Jan 23, 2022 •

edited

Loading

mkhon Jan 24, 2022

JackBoosY commented Jan 25, 2022

ras0219-msft left a comment •

edited

Loading

mkhon commented Jan 29, 2022

mkhon commented Jan 29, 2022 •

edited

Loading

JackBoosY commented Feb 6, 2022

JackBoosY commented Feb 21, 2022

JackBoosY commented Mar 18, 2022

JackBoosY commented May 5, 2022

BillyONeal commented May 6, 2022

JackBoosY commented May 6, 2022

mkhon commented May 7, 2022

JackBoosY commented May 9, 2022

JackBoosY commented May 13, 2022

BillyONeal commented May 16, 2022 •

edited

Loading

JackBoosY commented May 18, 2022

mkhon commented May 31, 2022

mkhon commented Jun 2, 2022

JackBoosY commented Jun 6, 2022

mkhon commented Jun 6, 2022

JackBoosY commented Jun 8, 2022

ras0219-msft commented Jun 10, 2022

dg0yt commented Jun 10, 2022

[vcpkg_execute_*_process] Add support for build profiling #22676

[vcpkg_execute_*_process] Add support for build profiling #22676

Conversation

mkhon commented Jan 21, 2022 • edited Loading

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?

Neumann-A commented Jan 21, 2022

mkhon commented Jan 21, 2022

dg0yt Jan 22, 2022

Choose a reason for hiding this comment

mkhon Jan 23, 2022

Choose a reason for hiding this comment

dg0yt Jan 23, 2022 • edited Loading

Choose a reason for hiding this comment

dg0yt Jan 22, 2022

Choose a reason for hiding this comment

mkhon Jan 23, 2022

Choose a reason for hiding this comment

dg0yt Jan 23, 2022 • edited Loading

Choose a reason for hiding this comment

mkhon Jan 24, 2022

Choose a reason for hiding this comment

JackBoosY commented Jan 25, 2022

ras0219-msft left a comment • edited Loading

Choose a reason for hiding this comment

mkhon commented Jan 29, 2022

mkhon commented Jan 29, 2022 • edited Loading

JackBoosY commented Feb 6, 2022

JackBoosY commented Feb 21, 2022

JackBoosY commented Mar 18, 2022

JackBoosY commented May 5, 2022

BillyONeal commented May 6, 2022

JackBoosY commented May 6, 2022

mkhon commented May 7, 2022

JackBoosY commented May 9, 2022

JackBoosY commented May 13, 2022

BillyONeal commented May 16, 2022 • edited Loading

JackBoosY commented May 18, 2022

mkhon commented May 31, 2022

mkhon commented Jun 2, 2022

JackBoosY commented Jun 6, 2022

mkhon commented Jun 6, 2022

JackBoosY commented Jun 8, 2022

ras0219-msft commented Jun 10, 2022

dg0yt commented Jun 10, 2022

mkhon commented Jan 21, 2022 •

edited

Loading

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

dg0yt Jan 23, 2022 •

edited

Loading

dg0yt Jan 23, 2022 •

edited

Loading

ras0219-msft left a comment •

edited

Loading

mkhon commented Jan 29, 2022 •

edited

Loading

BillyONeal commented May 16, 2022 •

edited

Loading