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

[CMake] VCPKG_APPLOCAL_DEPS sometimes causes conflicts when processing multiple files in the same directory #13025

Merged

Conversation

sandercox
Copy link
Contributor

In our CMake setup we often have multiple targets output to the same directory keeping lists of plugins together for instance. When these targets run the custom target on windows to gather their depedencies (VCPKG_APPLOCAL_DEPS is on) the custom target script sometimes complains that files are already in use.

Adding a USES_TERMINAL makes sure these commands do on run in parallel. Maybe there is another (better) way to only do when running within the same directory so in 'normal' situation these can still happen in parallel

…is run in parallel so make it uses_terminal to allow only 1 job to run simultaneously
@JackBoosY JackBoosY added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed requires:discussion labels Aug 21, 2020
@ras0219
Copy link
Contributor

ras0219 commented Aug 21, 2020

... the custom target script sometimes complains that files are already in use.

In this specific case, it should then be safe to skip over files that are in use (because the script using them will handle their transitive dependencies). However, that may not be good enough to solve the general problem.

I'm a bit concerned about USES_TERMINAL potentially making the user's link steps serialized; I haven't checked recently, but I recall this essentally adding an && powershell at the end of the link command, which means that the whole line would be marked as USES_TERMINAL.

Another approach that might be of interest would be to build on top of the applocal.ps1 changes in #12223 and group all the applocal commands into a single call. This could have some nice performance benefits since invoking powershell is somewhat expensive.

@sandercox
Copy link
Contributor Author

I'm a bit concerned about USES_TERMINAL potentially making the user's link steps serialized; I haven't checked recently, but I recall this essentally adding an && powershell at the end of the link command, which means that the whole line would be marked as USES_TERMINAL.

Hmm that is a good point that I didn't think about. I'll investigate this a bit further and also look into your suggestion for another approach.

@sandercox
Copy link
Contributor Author

sandercox commented Aug 30, 2020

I'm a bit concerned about USES_TERMINAL potentially making the user's link steps serialized; I haven't checked recently, but I recall this essentially adding an && powershell at the end of the link command, which means that the whole line would be marked as USES_TERMINAL.

I did some tests to see if USES_TERMINAL affects the amount of targets that are linked in parallel, but just removing the linked output on some simple test targets and this seems to work just fine. But, atleast for Ninja generator, USES_TERMINAL will cause all post build steps to be run single threaded. So if a user adds other steps they might conflict.

I've also been looking at the other suggestion of combining and making sure it only runs once. Would you have any pointers as in how to make that happen. Right now we can only know when a target needs this on the add_executable call. And the actual dependencies are not soley determined by the CMake script. Eg. if the binary doesn't use a particular DLLs code that was specified using target_link_library it will just not list in the dumpbin.
So we would then need some dummy target that executes always even when you just compile a single target. I have no idea how to accomplish this in CMake - so I'd need some pointers.

Then again, I do not think that the USES_TERMINAL is as bad as we were afraid of.

I did change the code slightly to make this an optional setting.

…S_SERIALIZED

This clarifies it as an experimental flag that may change or be removed at any time
@ras0219-msft
Copy link
Contributor

ras0219-msft commented Sep 24, 2020

Thanks for making it optional -- I think the north star obviously needs to be thread safe all the time (possibly via merging them together), so I named the option accordingly to indicate to users that they shouldn't depend on this behavior.

That said, this seems like a good intermediate solution until we're able to do the perfect one.

@JackBoosY
Copy link
Contributor

Blocked by #13722.

@sandercox
Copy link
Contributor Author

#13722 seems to be merged so can this get merged too?

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@JackBoosY
Copy link
Contributor

Looks good, all test passed.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Oct 9, 2020
@dan-shaw
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal BillyONeal merged commit e93810d into microsoft:master Oct 20, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution!

@sandercox sandercox deleted the VCPKG_APPLOCAL_DEPS-not-parallel branch October 20, 2020 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants