Add subdirectories to source file collection logic in SConstruct (reverted)

[Chkoupinator]

This adds sub-directory detection when gathering sources for SConstruct, it took me a while to figure out why using subfolders in my src folder wasn't working for me to realize that Glob simply does not look at subfolders.

I think this would improve the template for people who want to use it for a serious project as I find it very unlikely that anyone would put all their files on the root src/ folder for anything remotely big

[Ivorforce]

Hello, thank you for opening a PR!
This part of the code is a little bit tricky. At the very least, "variant directory" features of SCons will require sources in the right format.
I get your problem though - not being able to put .cpp files into subfolders, and having to find out why, isn't exactly a nice first impression.

An os.walk implementation could be adopted, for example, one was suggested in 2012 in this StackOverflow thread.

However, most projects are not arbitrarily deep. In my own project, I prefer to just support a 3-deep hierarchy with SCons supported functionality, like suggested here: https://stackoverflow.com/a/10599984

[Chkoupinator]

thanks for the feedback!
I updated it, this could probably be improved by adding a layers of depth parameter but this is better than nothing imo

[Ivorforce]

Seems fine to me; this should be good enough for most projects, and it's easy to understand.

[Chkoupinator]

@Ivorforce do you know what the process would be to make this PR move forward?

[paddy-exe]

@Ivorforce do you know what the process would be to make this PR move forward?

That would be my merge. Apologies for the delay.

I am not sure if we shouldn't directly just go for the os.walk implementation you linked @Ivorforce . This feels more like a band aid solution to me even if probably most people will indeed only use max 3 subdirectories.

[Chkoupinator]

@paddy-exe no worries! my initial implementation used os.walk. Let me know if I should revert my last commit

[paddy-exe]

@paddy-exe no worries! my initial implementation used os.walk. Let me know if I should revert my last commit

Thank you. After further discussing on RocketChat with more maintainers, let's go for the explicit subdirectories (the current implementation).

However, could you please squash the two commits into one (and clean up the commit descriptions)?

[Chkoupinator]

done! @paddy-exe

[paddy-exe]

Merged! Thank you for your contribution!

[doZennn]

This seems to be break linking if you have generated docs in src\gen\:

src\gen\doc_data.gen.windows.template_debug.dev.x86_64.obj : warning LNK4042: object specified more than once; extras ignored
LINK : error LNK1218: warning treated as error; no output file generated

[paddy-exe]

This seems to be break linking if you have generated docs in src\gen\:

src\gen\doc_data.gen.windows.template_debug.dev.x86_64.obj : warning LNK4042: object specified more than once; extras ignored

LINK : error LNK1218: warning treated as error; no output file generated

Would you mind creating an issue for this and link this PR?

[Ivorforce]

Oh right, I totally forgot that doc_data.cpp exists 😅 sorry!

The solution in my own code is the following:

sources.extend([
    f for f in env.Glob("src/*.cpp") + env.Glob("src/*/*.cpp") + env.Glob("src/*/*/*.cpp")
    # Generated files will be added selectively and maintained by tools.
    if not "/gen/" in str(f.path)
])

However, this is dangerously close to being complicated again, so I'd also be open to using the previously suggested os.walk.

(I suppose this is why David prefers using explicitly adding all source files instead 😆)

[Chkoupinator]

I should have a local history of the first implementation I had proposed, I had thought of docs when making it but forgot about them when implementing the version that ended up being merged 😅 (I don't have docs in my project so I didn't test it 😅 😅 )

I'll make a PR for it when I get back home

[Ivorforce]

While we're waiting to agree on a different solution, I reverted the commit (directly on main) because I saw people start having issues on Discord.

[Chkoupinator]

I finally found some time to get back to this, sorry for the very long delay, you can find the new pull request with the original os.walk implementation in the PR #102 @Ivorforce @paddy-exe