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.

Sign up for GitHub

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

Jump to bottom

[vcpkg/msbuild] parellel build #19718

Merged
merged 9 commits into from
Sep 30, 2021
Merged

[vcpkg/msbuild] parellel build #19718

merged 9 commits into from
Sep 30, 2021

Conversation

mheyman
Copy link
Contributor

@mheyman mheyman commented Aug 23, 2021

Describe the pull request

Defaults to parallel build for msbuild projects. Prior, only projects built in parallel. With this change, source files within the projects can build in parallel. Ports like activemq-cpp use one vcxproj file for the entire port so without this change, it will build single threaded.

  • Which triplets are supported/not supported?

N/A

  • 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?

N/A

@autoantwort
Copy link
Contributor

autoantwort commented Aug 23, 2021 •
edited
Loading

It is maybe possible to respect VCPKG_MAX_CONCURRENCY?

@JackBoosY JackBoosY added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Aug 24, 2021
@JackBoosY
Copy link
Contributor

It is maybe possible to respect VCPKG_MAX_CONCURRENCY?

I think that's the correct way instead of this changes.

@mheyman
Copy link
Contributor Author

mheyman commented Aug 24, 2021

A few lines below the change the /m switch already existed. That switch uses up to the max number of CPUs to parallelize vcxproj builds within a solution, not .cpp builds within a project. That switch could be changed to /m:${VCPKG_MAX_CONCURRANCY} but I'm not sure if the multi-tooltask which the /p:UseMultiToolTask parameter invokes respects that value. I'm looking for the source to inspect it but have yet to find it...

@mheyman
Copy link
Contributor Author

mheyman commented Aug 24, 2021

The latest change assumes VCPKG_MAX_CONCURRENCY turns into a CMake variable somehow - I don't know how to use VCPKG_MAX_CONCURRENCY and I know that, in general, passing environment variables though vcpkg requires a bit of complication

@mheyman
Copy link
Contributor Author

mheyman commented Aug 24, 2021 •
edited
Loading

So, it looks like the VCPKG_MAX_CONCURRENCY environment variable gets turned into the VCPKG_CONCURRENCY CMake variable! (through a call to get_concurrency().

I think these changes fully respect VCPKG_MAX_CONCURRENCY.

@mheyman
[vcpkg/msbuild] EnforceProcessCountAcrossBuilds
e0cd505 
EnforceProcessCountAcrossBuilds=true (must have value)
@mheyman
Copy link
Contributor Author

mheyman commented Aug 24, 2021

Just verified it respects VCPKG_MAX_CONCURRENCY using Process Explorer!

@JackBoosY JackBoosY requested a review from ras0219-msft August 25, 2021 05:49
@JackBoosY
Copy link
Contributor

I hope my colleague will review this modification further because I don't have enough knowledge about msbuild.

BillyONeal
BillyONeal reviewed Sep 30, 2021
View reviewed changes
scripts/cmake/vcpkg_install_msbuild.cmake
@@ -143,7 +143,10 @@ function(vcpkg_install_msbuild)
"/p:VcpkgTriplet=${TARGET_TRIPLET}"
"/p:VcpkgInstalledDir=${_VCPKG_INSTALLED_DIR}"
"/p:VcpkgManifestInstall=false"
"/m"
"/p:UseMultiToolTask=true"
Copy link
Member

Choose a reason for hiding this comment

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

What effect does this have, if any, for older Visual Studios without MultiToolTask?

@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 30, 2021
@BillyONeal BillyONeal added the info:reviewed Pull Request changes follow basic guidelines label Sep 30, 2021
@BillyONeal
Copy link
Member

/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 BillyONeal merged commit 7e5cfcc into microsoft:master Sep 30, 2021
@BillyONeal
Copy link
Member

Thanks :)

@mheyman mheyman mentioned this pull request Oct 3, 2021
Merged
@BillyONeal BillyONeal mentioned this pull request Oct 4, 2021
Merged
@mheyman mheyman deleted the msbuild_parallel branch August 22, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal approved these changes

@github-actions github-actions[bot] github-actions[bot] approved these changes

@ras0219-msft ras0219-msft Awaiting requested review from ras0219-msft

Assignees

@JackBoosY JackBoosY

Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mheyman @autoantwort @JackBoosY @BillyONeal