Skip to content

Build: fix pre-compiled header files for MSVC/Ninja#3204

Merged
pixar-oss merged 3 commits intoPixarAnimationStudios:devfrom
mistafunk:ninja-msvc-no-pch
Nov 26, 2024
Merged

Build: fix pre-compiled header files for MSVC/Ninja#3204
pixar-oss merged 3 commits intoPixarAnimationStudios:devfrom
mistafunk:ninja-msvc-no-pch

Conversation

@mistafunk
Copy link
Copy Markdown

@mistafunk mistafunk commented Aug 3, 2024 •
edited
Loading

Description of Change(s)

  • Fix use of pre-compiled header when using MSVC with Ninja
  • Fix potential compilation errors with Ninja caused by concurrent writes to PDB files when building with multiple cores.
  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

meshula
meshula reviewed Aug 3, 2024
View reviewed changes
cmake/defaults/Options.cmake
@@ -72,7 +72,7 @@ option(PXR_ENABLE_GL_SUPPORT "Enable OpenGL based components" ON)

# Precompiled headers are a win on Windows, not on gcc.
Copy link
Copy Markdown
Member

@meshula meshula Aug 3, 2024

Choose a reason for hiding this comment

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

can we also revise the comment to not say Windows, but rather "msvc with msbuild"

Copy link
Copy Markdown
Author

@mistafunk mistafunk Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe just remove this comment - or do the homework and measure this properly :-)

I did a quick stopwatch comarison... msvc/ninja with PCH took about 5min and without PCH about 6min... so we could also just say "... are a win with MSVC ..."

Copy link
Copy Markdown
Contributor

@pmolodo pmolodo Aug 5, 2024

Choose a reason for hiding this comment

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

On another note - I've been using sccache with windows msvc USD builds recently, and been pretty happy with it so far. Haven't done any time comparisons, but rebuilds feel faster - particularly if you switch branches and come back, so a lot of stuff has updated timestamps but hasn't actually changed.

@meshula
Copy link
Copy Markdown
Member

meshula commented Aug 3, 2024

thanks for noticing this

Simon Haegler added 2 commits August 4, 2024 15:27
Build (msvc/ninja): pre-compiled header bug fix: make sure the direct…
f2ee21f 
…ory for the output pch file exists
Build (msvc/ninja): pre-compiled header bug fix: make sure to set fil…
4f089c3 
…e properties AFTER adding them to the target

otherwise the custom command is not triggered
@mistafunk
Copy link
Copy Markdown
Author

mistafunk commented Aug 4, 2024 •
edited
Loading

thanks for noticing this

You are welcome. I took another look and found the actual bugs. There was a missing directory and wrong cmake command order when PCH was enabled. I reused this PR and force pushed.

@mistafunk
Copy link
Copy Markdown
Author

mistafunk commented Aug 4, 2024

A note for CLion users, do not get confused (like I did) by output like this in the CMake window:

Problems were encountered while collecting compiler information:
	C:\Users\<xxx>\AppData\Local\Temp\compiler-file7019753171835842146: fatal error C1083: Cannot open include file: 'pxr/usd/_ar/pypch.h': No such file or directory

This is an artifact of the CLion "intellisense" and not related to the USD build system...

@mistafunk mistafunk changed the title Build: disable PCH by default if MSVC and Ninja generator Build: fix pre-compiled header files for MSVC/Ninja Aug 4, 2024
@mistafunk mistafunk requested a review from meshula August 4, 2024 14:32
@meshula
Copy link
Copy Markdown
Member

meshula commented Aug 5, 2024

Thanks, this looks ready for an internal review.

@mistafunk
Copy link
Copy Markdown
Author

mistafunk commented Aug 5, 2024

Btw, I tried to run the tests, there are over 100 failing out of 600 - is this exepected on dev?

@jesschimein
Copy link
Copy Markdown
Collaborator

jesschimein commented Aug 5, 2024

Filed as internal issue #USD-9931

@jesschimein
Copy link
Copy Markdown
Collaborator

jesschimein commented Aug 5, 2024

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Aug 5, 2024

Azure Pipelines successfully started running 1 pipeline(s).

@sunyab
Copy link
Copy Markdown
Contributor

sunyab commented Aug 5, 2024

@mistafunk Our internal CI is currently showing no failures, so I would not expect anything like that. I also can't imagine anything specific to this change that would be causing that either.

Do you have an example of a failing test you could share here? I'd bet they're all failing for the same reason.

@mistafunk mistafunk mentioned this pull request Aug 5, 2024
Closed
2 tasks
@pmolodo
Copy link
Copy Markdown
Contributor

pmolodo commented Aug 5, 2024

I'll close my PR3141, since it looks like this fixes that and does a bit more as well!

@mistafunk
Copy link
Copy Markdown
Author

mistafunk commented Aug 7, 2024

I cannot reproduce the linux build failure locally... is the pipeline timeout too low?

@DDoS
Copy link
Copy Markdown
Contributor

DDoS commented Aug 8, 2024

There is also #3120 that fixes this same issue.

@jimwang118 jimwang118 mentioned this pull request Oct 30, 2024
Merged
7 tasks
@sunyab sunyab mentioned this pull request Nov 8, 2024
Closed
@pixar-oss pixar-oss merged commit b677b23 into PixarAnimationStudios:dev Nov 26, 2024
@adskWangl
Copy link
Copy Markdown
Contributor

adskWangl commented Nov 26, 2024

There is also #3120 that fixes this same issue.

Thank you and we'll close #3120 as that is for the same issue.

@pixar-oss pixar-oss mentioned this pull request Nov 26, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@meshula meshula Awaiting requested review from meshula

1 more reviewer

@pmolodo pmolodo pmolodo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@mistafunk @meshula @jesschimein @sunyab @pmolodo @DDoS @adskWangl @pixar-oss