-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Improve CMake for IDE Projects (Visual Studio) #13704
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
Merged
slouken
merged 7 commits into
libsdl-org:main
from
playmer:playmer/cmake_ide_project_improvement
Aug 14, 2025
+484
−107
Merged
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f50766c
Start tracking SDL internal headers and add headers to SDL targets, a…
playmer 89009d4
Two globs got duplicated in the last commit.
playmer 3f28eb0
Filter out paths that aren't a prefix to the PROJECT_SOURCE_DIR, only…
playmer d0269b4
Should've gotten 99% of stuff.
playmer 3c13253
Fix psp
playmer ddced75
Fix windows.
playmer d994e81
Seems when I was adjusting rwlock I made the same c -> h problem on a…
playmer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
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.
There's some more
sdl_glob_sources
underif(WINDOWS)
.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.
Sorry for the delay. wasn't able to tinker on this for most of the day. Looking at it more closely, there's actually a ton more that I've missed. Now I wonder if I should change approach and augment sdl_glob_sources to get the directories of the files passed in and add additional
*.h
globs in. What do you think? It'd certainly be less work to maintain it, (For however much work it really is to add a few extra lines of CMake to account for headers is.) Though it might be better for it to accept directories rather than files...I dunno!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'd rather not make the globs too smart.
We're already not following cmake best practices by not explicitly listing the sources.
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.
Yes, I do much prefer being explicit, but I know that doesn't work for everyone. I'll do a thorough pass tomorrow and try not to miss any.
As a tangential question, is there a reason the GLOBs aren't using
CONFIGURE_DEPENDS
?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 didn't know about that option.
If performance does not suffer, it makes sense to use it.
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.
Oh sorry, I got caught up working on my
SDL_MenuBar
prototype, my plan was to doCONFIGURE_DEPENDS
as a separate change so I didn't end up prioritizing it. I can work on that tonight!Uh oh!
There was an error while loading. Please reload this page.
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.
A separate PR is OK as well. I was just checking in.
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.
Did some testing locally, I'm not noticing anything that I would be able to consistently nail down without just running builds over and over again. I pushed it up to CI and while it wasn't faster than the previous run on the branch, it was faster than a different run on the same commit.
Previous Commit Run 1: https://github.com/playmer/SDL-1/actions/runs/16858095731
Previous Commit Run 2: https://github.com/playmer/SDL-1/actions/runs/16857983229
CONFIGURE_DEPENDS Commit: https://github.com/playmer/SDL-1/actions/runs/16953366708
The flag should mostly affect incremental builds, as CMake needs to insert globs to run before building your code. I did a quick run of a rebuild by inserting blank lines in
SDL.h
and again found nothing notable:(Omitted the actual cmake/build output, but the full logs are here: https://gist.github.com/playmer/7d2c0bc75d694e582c08601f23f5b72b)
With Changes:
Without changes:
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 can push the change to this branch or not, don't want to make the decision for you! Just let me know what you'd prefer!
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.
Let's do it in a separate PR and let this one focus on making IDE's pretty again.