-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Improve PCH and enable PCH for Catch2 and tests #2894
Improve PCH and enable PCH for Catch2 and tests #2894
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2894 +/- ##
==========================================
+ Coverage 43.65% 44.13% +0.47%
==========================================
Files 253 254 +1
Lines 20940 21288 +348
Branches 5139 5214 +75
==========================================
+ Hits 9142 9395 +253
- Misses 10785 10906 +121
+ Partials 1013 987 -26 see 125 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
05109fc
to
617a786
Compare
617a786
to
524d8b9
Compare
Fails to build for me on Windows with VS2022:
|
@eXpl0it3r: hmm... it's concerning that the CI didn't catch that, to be honest. I wonder why... I will try to reproduce locally, I guess it's some headers getting copied too late |
Are we running any build with tests AND PCH enabled in the CI? |
524d8b9
to
ba39ec3
Compare
Duh, I always forget we are not. We should probably add a job or two... |
# necessary to find SFML headers from Catch2 through the PCH header | ||
target_include_directories(Catch2 PRIVATE ${PROJECT_SOURCE_DIR}/include) | ||
|
||
# necessary to find Catch2 headers from SFML through the PCH header | ||
target_include_directories(sfml-system PRIVATE ${CATCH2_INCLUDE_DIRS}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eXpl0it3r: this seems to solve the build on Windows, if you have any suggestions on a more elegant solution I'm all ears :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there isn't some include directory variable thingy from the target we can use. Maybe @ChrisThrasher has an idea.
Win 11 / VS 2022 (no Ninja or parallelization) Before this change: 1min 4s (Very scientifically measured with a stop watch 😄) |
//////////////////////////////////////////////////////////// | ||
|
||
#ifdef SFML_BUILD_TEST_SUITE | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just pass the path to this header to the target_precompiled_headers call within test/CMakeLists.txt? Feels out of place to add non-SFML headers to PCH.hpp. As seen it’s causing downstream complications which I’d rather we avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean to directly pass catch_test_macros
instead of PCH.hpp
when building Catch?
That was my first approach, but I noticed that Catch2 shares quite a lot of headers in common with SFML (e.g. ostream
, algorithm
, vector
, memory
), so it made sense to reuse the existing PCH.hpp
rather than creating a new ad-hoc one for Catch2.
The other issue is that we want catch_test_macros
to also be part of SFML's own PCH because we use that PCH in our own test suite.
So keeping everything in the same PCH seemed the most reasonable choice here.
We'd still have to keep catch2/catch_test_macros
in SFML
's own PCH to speed up compilation of our test drivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can’t you have multiple PCH sets? And then reuse the PCH set for the core library alongside the PCH for catch_test_macros?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can’t you have multiple PCH sets? And then reuse the PCH set for the core library alongside the PCH for catch_test_macros?
I don't see how that solves the issue, it would limit PCH reuse unless I am mistaken...
There are basically three categories of targets:
- SFML library: needs PCH_STDLIB + PCH_SFML
- SFML tests: needs PCH_STDLIB + PCH_SFML + PCH_CATCH
- Catch2 library: needs PCH_STDLIB + PCH_CATCH
Right now, my PCH is PCH_STDLIB + PCH_SFML + PCH_CATCH. This allows me to reuse the same PCH for all three categories of targets.
I definitely want "SFML library" and "SFML tests" to reuse the same PCH, to avoid recompiling two different PCHs -- but that requires that PCH to have access to Catch2 headers anyway.
If we were to go with your solution, then we'd have a few PCH files:
PCHStdlib.hpp
PCHStdlibSFML.hpp
(includesPCHStdlib.hpp
)PCHStdlibCatch.hpp
(includesPCHStdlib.hpp
)PCHStdlibSFMLCatch.hpp
(includePCHSFML.hpp
andPCHCatch.hpp
)
- SFML libraries would use
PCHStdlibSFML.hpp
. - SFML tests would use
PCHStdlibSFMLCatch.hpp
. - Catch would use
PCHStdlibCatch.hpp
.
I would be able to use PCHStdlibSFML.hpp
for SFML libraries, and use REUSE_FROM
from sfml-system
.
But if I had to use PCHStdlibSFMLCatch.hpp
for tests I would lose the ability to REUSE_FROM
from sfml-system
, which is a step backwards compared to the current approach. The same also applies to SFML examples.
I don't think going down this route would be better... in the end PCHs are an optimization of build times which already heavily screws up header hygiene, I think it's acceptable to use a few target_include_directories
to make everything work when PCHs are enabled.
…h_improvements_and_catch2_support
Pull Request Test Coverage Report for Build 8954073498Details
💛 - Coveralls |
@ChrisThrasher: any feedback on this? It makes a significant difference on my machine |
Sorry, but I'm not in favor of this PR. I think the complexity it adds outweighs whatever benefits it may have. A clean build of the test suite (including Catch2) takes under 10 seconds on my 2021 laptop. Adding complexity to improve compile time for users is one thing but I'm much less motivated to add complexity to improve compile times for the small number of people, myself included, who are building the tests on a regular basis. |
What exactly do you find most complicated about this PR, and what do you think would need to be simplified in order for you to consider approving it? |
I can't picture a way of transforming this PR into something I'd be willing to approve |
I think it's quite sad to ignore these sort of improvements. I also think we should start using PCH as our default in CI jobs and only have one or two sanity-check non-PCH jobs to save time and computing power (let's be green!). Anyone else on my side? @eXpl0it3r @Bromeon @binary1248 |
tl;dr:
-DSFML_BUILD_TEST_SUITE=1 -DSFML_ENABLE_PCH=0
-DSFML_BUILD_TEST_SUITE=1 -DSFML_ENABLE_PCH=1
Full analysis logs available here: