Skip to content
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

SCons: Replace _find_scu_section_name function with SCons builtins #98888

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

dustdfg
Copy link
Contributor

@dustdfg dustdfg commented Nov 6, 2024

Not only that it is a clean up and ammending of custom workaround but it also transparently allows to process SCons specific paths which contains # in the starts (which in turn in the future should allow me to implement SCU for thirdparty sources which should greatly increase compilation speed)

@dustdfg dustdfg marked this pull request as ready for review November 6, 2024 08:38
@dustdfg dustdfg requested a review from a team as a code owner November 6, 2024 08:38
Signed-off-by: Yevhen Babiichuk (DustDFG) <dfgdust@gmail.com>
@AThousandShips AThousandShips added this to the 4.x milestone Nov 6, 2024
@Repiteo Repiteo modified the milestones: 4.x, 4.4 Nov 6, 2024
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More native SCons implementations are always a plus!

@akien-mga akien-mga changed the title Replace _find_scu_section_name function with SCons builtins SCons: Replace _find_scu_section_name function with SCons builtins Nov 6, 2024
@dustdfg
Copy link
Contributor Author

dustdfg commented Nov 7, 2024

I've just found that scu_builders.py have a Ctrl+C, Ctrl+V version of method I am deleting in this PR. I think this should be done in this PR too. I've also noticed tendency not to use SCons related stuff inside builders... And when I tried to add from misc.utility.scons_hints import * to scu_builders.py it didn't work 😅 I delayed that question as much as possible but it is time to ask. Why does builders doesn't use SCons? Or if they use where and how...

@akien-mga
Copy link
Member

akien-mga commented Nov 7, 2024

Why does builders doesn't use SCons?

Usually just because the author of the builder was more familiar with Python than the SCons API. Cleaning things up by making use of SCons features is usually a good change.

@dustdfg
Copy link
Contributor Author

dustdfg commented Nov 7, 2024

I've just found that scu_builders.py have a Ctrl+C, Ctrl+V version of method I am deleting in this PR.

Oh. I was a bit wrong. It returns in format dir_dir_dir instead of dir/dir/dir so it is for another PR 🤔 or for this... Anyway this PR again can be considered as ready to be merged 😅

@Repiteo Repiteo merged commit ca81cd3 into godotengine:master Nov 7, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 7, 2024

Thanks!

@dustdfg dustdfg deleted the scu_rely_on_scons branch November 7, 2024 18:43
dustdfg added a commit to dustdfg/godot that referenced this pull request Dec 7, 2024
Followup to godotengine#98888. Note here can't be any regressions like godotengine#99607
because here separator is not OS dependent and set explicitly

Signed-off-by: Yevhen Babiichuk (DustDFG) <dfgdust@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants