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

[python3] Enable parallel builds; use vcpkg_install_msbuild() #12223

Closed
wants to merge 20 commits into from

Conversation

ras0219
Copy link
Contributor

@ras0219 ras0219 commented Jul 2, 2020

See Title

@JackBoosY JackBoosY added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Jul 3, 2020
@PhoebeHui PhoebeHui added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist and removed category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed labels Jul 3, 2020
@PhoebeHui
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Jul 6, 2020
@strega-nil
Copy link
Contributor

@ras0219 could you merge with master?

@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ras0219
Copy link
Contributor Author

ras0219 commented Aug 11, 2020

After further work on this, it seems that /p:CL_MPCount is not effective.

However, I think that using the CL environment variable is appropriate for this specific case for two reasons:

  1. It is common practice elsewhere to specify flags via environment variables (CXXFLAGS)
  2. This flag is a CL-specific flag -- if some other compiler was to be used inside msbuild, it is desirable for it to be ignored. Therefore, the CL environment variable functions perfectly for this use.

While testing, I found two additional significant performance issues:

  1. file(COPY) is ~3x slower than xcopy for duplicating the source tree.
  2. Spawning ~20 powershell.exe processes takes a long time during vcpkg_copy_tool_dependencies()

After addressing each of these points, the reported time for vcpkg build python3:x64-windows on my machine is down to 38.25s from 1.331mins -- ~2x faster.

@BillyONeal
Copy link
Member

However, I think that using the CL environment variable is appropriate for this specific case for two reasons:

  1. It is common practice elsewhere to specify flags via environment variables (CXXFLAGS)

I would object much less to CL rather than _CL_. The former I've seen used in build systems sometimes, particularly make derived ones. The latter is the escape hatch for the former.

@ras0219-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal
Copy link
Member

Dropping 'reviewed' since it's been changed a lot since then

@ras0219
Copy link
Contributor Author

ras0219 commented Aug 26, 2020

This is blocked on fixes in #13019 and #13126

@ras0219-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ras0219 ras0219 marked this pull request as draft September 1, 2020 05:13
@ras0219-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@PhoebeHui
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants