build: minor refactoring and fixes #112
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Could you also add the missing |
|
Is it needed? Edit: oh, the library is used without even including the proper headers... Fixing it now |
Tachi107
force-pushed
the
build-minor-fixes
branch
from
5386603
to
2ecbe53
Compare
|
|
|
I replied to this two hours ago, but GitHub apparently lost my email... I said:
Yeah, but |
Tachi107
force-pushed
the
build-minor-fixes
branch
3 times, most recently
from
f0b6aae
to
3c61593
Compare
|
This is now ready to be merged; I reverted the |
|
I assure you it's needed. |
Oh ok, thanks for the info. As I don't have access to my computer right now, could you include here the build failure? Also, the fact that the CemuAudio doesn't link to CemuCommon but uses it's precompiled headers is a bit odd, I should also look into it. I'll do it in a separate PR though, this one can be merged as is. |
This precompiled header seems very fishy. |
|
Yeah, I guess that this happens because, as far as I can tell, the current use of Currently, CemuCommon exposes a precompiled header as part of its public API, and while this is discouraged in CMake's documentation I think that in this particular case this is fine, as CemuCommon is an internal target. At the same time, every other CemuXxx target uses There are two possible solutions:
I would personally prefer the second option, as it is more explicit and leads to less intertwined targets. See the CMake docs for details: https://cmake.org/cmake/help/latest/command/target_precompile_headers.html |
Tachi107
force-pushed
the
build-minor-fixes
branch
from
3c61593
to
0581b46
Compare
|
@abouvier could you please check if the latest commit fixes your issue? |
Tachi107
force-pushed
the
build-minor-fixes
branch
from
0581b46
to
8bc4d43
Compare
Tachi107
force-pushed
the
build-minor-fixes
branch
from
89fc9bf
to
d3844a6
Compare
Closed
Nope, it's worse: If I define And... again, I must define |
|
Woah, it seems that GCC is even pickier when it comes to precompiled
headers; you have to use pretty much the same exact set of compiler
flags for both compiling the headers and consuming them...
…--
OpenPGP key: 66DE F152 8299 0C21 99EF A801 A8A1 28A8 AB1C EE49
|
|
It's working now with all the |
Tachi107
force-pushed
the
build-minor-fixes
branch
from
9590953
to
bd52146
Compare
|
This should be now ready, for real this time. I've never used precompiled headers myself, but by looking at CMake's documentation, CI status and @abouvier's feedback it seems that this is the right way™ to do it. Please do not squash the branch on merge, otherwise all the information in the commit message will get lost. |
|
Sadly I'm not sure the header is compiled only once: And each |
Yes, I was wrong. CMake's docs aren't super clear. Anyway the precompiled headers were already part of CemuCommon's PUBLIC interface, so this change does at least fix some issues (like yours) and make the code more concise (by removing the need of calling
Woah, they're big. Were they this big even before this patch? Edit: I've force-pushed yet another time to fix the commit message and to reduce the diff by a few lines (as mentioned in #117 (comment)) |
Tachi107
force-pushed
the
build-minor-fixes
branch
from
bd52146
to
76ffc4d
Compare
Yeah, same size since the first commit. And the precompiled header breaks ccache, so every rebuild is like a first build :$ |
- Fix target_precompile_headers() usage; the CemuCommon target exposes the src/Common/precompiled.h precompiled header as part of its public interface with target_precompile_headers(CemuCommon PUBLIC precompiled.h), so all the other targets wanting to use the precompiled header have to link to the CemuCommon target with target_precompile_headers(TargetName PRIVATE CemuCommon). - Set the project version to 2.0 - Set RUNTIME_OUTPUT_DIRECTORY instead of only their _DEBUG and _RELEASE variants, fixing the compilation when neither build types are defined - Use a consistent indentation style (tabs, like in the .cpp files) - Use "modern" variants of some functions, e.g. add_definitions -> add_compile_definitions
Tachi107
force-pushed
the
build-minor-fixes
branch
from
76ffc4d
to
719ee90
Compare
|
Rebased again on the main branch |
|
@abouvier I've now modified the code to use the first approach I
described earlier (i.e. linking everything to CemuCommon), could you
check if this fixes your GCC build?
Edit: GitHub sent these email replies after more than one month, for some reason...
…--
OpenPGP key: 66DE F152 8299 0C21 99EF A801 A8A1 28A8 AB1C EE49
|
This comment was marked as outdated.
This comment was marked as outdated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some bits originated from #75
target_precompile_headers()usage; the CemuCommon target exposes thesrc/Common/precompiled.hprecompiled header as part of its public interface withtarget_precompile_headers(CemuCommon PUBLIC precompiled.h), so all the other targets wanting to use the precompiled header have to link to the CemuCommon target withtarget_precompile_headers(TargetName PRIVATE CemuCommon).RUNTIME_OUTPUT_DIRECTORYinstead of only their_DEBUGand_RELEASEvariants, fixing the compilation when neither build types are defined (see Current State Of Linux Builds #1 (comment))add_definitions->add_compile_definitionsstd::enable_if& co instead ofboost::