Add subdirectories to source file collection logic in SConstruct using os.walk #102
Conversation
Chkoupinator
force-pushed
the
glob-oswalk-subfolders
branch
3 times, most recently
from
17afe03 to
28b6d74
Compare
|
Excuse the mess I couldn't recover the original implementation from my local history, it was gone for some reason, I recovered some of it from this commit's diff but a part of it got messed up and my local project isn't testing things thoroughly enough it seems. It should be good now |
Chkoupinator
force-pushed
the
glob-oswalk-subfolders
branch
from
47f9b9d to
ff86afb
Compare
SConstruct
Outdated
| sources = [] | ||
| # Recursively add every .cpp file in the src directory. | ||
| for folder_path, _, _ in os.walk("src"): | ||
| if not folder_path.endswith("gen"): # The doc data in src/gen is added later |
There was a problem hiding this comment.
This was supposed to be /gen, otherwise it will match folders like regen (i.e. any words that end with "gen").
| if not folder_path.endswith("gen"): # The doc data in src/gen is added later | |
| if not folder_path.endswith("/gen"): # The doc data in src/gen is added later |
There was a problem hiding this comment.
I'm wondering if the paths returned by os.walk always use the '/' separator or if they can use a '' separator on windows. I was writing a comment suggesting we use os.path.join("", "gen") instead
There was a problem hiding this comment.
or maybe os.sep + "gen"
There was a problem hiding this comment.
Good point. Then let's use if not "gen" in os.path.split(folder_path). That would also catch subfolders of gen folders (which might be a good default actually).
There was a problem hiding this comment.
(I just did a test on windows and it does use an "\" separator so it wouldn't work 😅 )
There was a problem hiding this comment.
alright makes sense, I personally tested an os.walk on windows 5 minutes ago and it returned \ separated paths so it's very much still a thing, I'll change it to os.path.split
There was a problem hiding this comment.
how does the gen folder behave when there are subfolders though? does it make the generated files in subfolders of src/gen or does it add /gen folders in the subfolders of src? or neither? if it's not the first option maybe it would be better to do if os.path.split(folder_path)[-1] != "gen"
There was a problem hiding this comment.
The gen folder doesn't "behave" like anything. We simply use it to dump the docs generated .cpp file in there, and godot-cpp adds it manually to the build sources.
Practically, most users won't do anything with their gen folder beyond that, so it doesn't matter what exactly we do. But I think not including auto-generated .cpp files is a sensible default, since they may or may not be re-generated depending by specific build specific options (with zombies of old versions floating around if not).
There was a problem hiding this comment.
yeah I realized that after posting it, how about doing if os.path.split(folder_path)[-1] != "gen" instead of "gen" not in os.path.split(folder_path) as the latter suggests gen could be anywhere in the path and not the last folder
There was a problem hiding this comment.
as the latter suggests gen could be anywhere in the path and not the last folder
Yes, that is intended. Like I said above, not auto-including anything in gen folders is probably a sensible default.
…g os.walk Added automatic file collection using os.walk so that source files in subdirectories of src/ get properly detected by SConstruct. Co-authored-by: Lukas Tenbrink <[email protected]>
Chkoupinator
force-pushed
the
glob-oswalk-subfolders
branch
from
ff86afb to
6f0b5bd
Compare
|
Is this PR ready to be merged? |
|
No, the final adjustment is still outstanding, and I would like to test it (this time 😅) before merge |
3 participants
This is a resubmit of #98 which was reverted due to the collection conflicting with the generated files for documentation.
This pull request proposes adding automatic file collection using os.walk so that source files in subdirectories of src/ get properly detected by SConstruct.