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

[Bindings] Build profile now strips methods and skip files #1675

Closed
wants to merge 2 commits into from

Conversation

Faless
Copy link
Contributor

@Faless Faless commented Dec 20, 2024

This allows removing dependencies that are not explicitly unused by the gdextension being built.

@Faless Faless added enhancement This is an enhancement on the current functionality needs testing topic:buildsystem Related to the buildsystem or CI setup labels Dec 20, 2024
@Faless Faless requested a review from a team as a code owner December 20, 2024 02:54
@Faless Faless force-pushed the build/profile_strip_methods branch from 83210b2 to 2d9f151 Compare December 20, 2024 03:06
@Zylann
Copy link
Collaborator

Zylann commented Dec 20, 2024

Tried with my project which only has Object, Resource and Image in its build profile:
First I got this error:

godot_cpp_fork\src\core\print_string.cpp(33): fatal error C1083: Cannot open include file: 'godot_cpp/classes/os.hpp': No such file or directory

Which I think was needed by error macros, which need to use the print function, which uses OS.
So I added OS to the profile.

After that, the project compiled successfully.

This is what got compiled (I only deleted the gen folder prior to testing):

gen\src\variant\packed_color_array.cpp
gen\src\classes\os.cpp
gen\src\classes\xml_parser.cpp
gen\src\classes\class_db_singleton.cpp
gen\src\variant\packed_float32_array.cpp
gen\src\variant\array.cpp
gen\src\variant\packed_string_array.cpp
gen\src\variant\packed_float64_array.cpp
gen\src\variant\packed_int64_array.cpp
gen\src\classes\semaphore.cpp
gen\src\classes\object.cpp
gen\src\variant\node_path.cpp
gen\src\variant\packed_int32_array.cpp
gen\src\classes\resource.cpp
gen\src\classes\ref_counted.cpp
gen\src\variant\string.cpp
gen\src\variant\packed_byte_array.cpp
gen\src\variant\string_name.cpp
gen\src\variant\dictionary.cpp
gen\src\classes\file_access.cpp
src\classes\low_level.cpp
gen\src\variant\rid.cpp
gen\src\variant\callable.cpp
gen\src\variant\packed_vector2_array.cpp
gen\src\variant\signal.cpp
gen\src\variant\packed_vector3_array.cpp
gen\src\variant\packed_vector4_array.cpp
src\core\print_string.cpp
gen\src\variant\utility_functions.cpp
src\variant\variant.cpp
gen\src\classes\worker_thread_pool.cpp
gen\src\classes\image.cpp

So that could fix #1674.

However I later noticed that if I run SCons again, it keeps compiling the same files again, even if I haven't changed any file. Shouldn't it compile only what changed? Is something breaking that detection?

@enetheru
Copy link
Contributor

I had a CMake solution working with build profiles prior to this, and now I get link errors. I need to investigate but Christmas is denying me the time. I'm hoping to investigate why, but it will take me a week at least.

@enetheru
Copy link
Contributor

I'm hoping to investigate why, but it will take me a week at least.

Turns out I had a moment, I can get it to compile by adding all the failed link classes to the json file.

@enetheru
Copy link
Contributor

Figured out what I needed to change, I didnt initially notice that the function signature for generate_bindings had to changed to include the build_profile. I have successful builds that pass the test project again once I adjusted.

Though I do receive a bunch of warnings from VS about files that are not created. I dont get such warnings from Ninja.

C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(254,5): warning MSB8065: Custom build for item "C:\build\godot-cpp\Windows-bprofile-msvc\gdextension\extension_api.json" succeeded, but specified output "c:\build\godot-cpp\windows-bprofile-msvc\cmake-build\gen\include\godot_cpp\classes\aes_context.hpp" has not been created. This may cause incremental build to work incorrectly. [C:\build\godot-cpp\Windows-bprofile-msvc\cmake-build\godot-cpp.editor.vcxproj]

This is of course because the generation logic has changed, and skips generating files for some predicates.

From what it looks like from git history, the print_file_list function was created to support binding generation from CMake such that it wont re-attempt generation if the files exist already. As files listed are not generated, it will trigger a re-gen every time, altering timestamps and causing an unnecessary re-build.

This might be the case in scons too, because I get a re-build everytime, where prior profile code would not.

This allows removing dependencies that are not explicitly unused by the
gdextension being built.
@Faless Faless force-pushed the build/profile_strip_methods branch from 2d9f151 to d2943a7 Compare December 30, 2024 15:58
@Faless
Copy link
Contributor Author

Faless commented Dec 30, 2024

Ah, indeed, I missed a change in get_file_list, should be fixed now.

@Faless
Copy link
Contributor Author

Faless commented Dec 30, 2024

Ah, indeed, I missed a change in get_file_list, should be fixed now.

We had a script to check the file list in CI (misc/scripts/check_get_file_list.py), but it's apparently not used anymore... not sure why...

We should re-add it, and probably the the build profile too with it.

@Faless Faless requested a review from a team as a code owner December 30, 2024 16:10
@Faless
Copy link
Contributor Author

Faless commented Dec 30, 2024

We should re-add it, and probably the the build profile too with it.

Done as a separate commit.

@enetheru
Copy link
Contributor

enetheru commented Dec 30, 2024

Confirmed with these changes I do not have to modify the build_profile.json, and I no longer get the warnings. Depending on the order of merge, I will update my CMake profile branch.

@Faless
Copy link
Contributor Author

Faless commented Jan 6, 2025

Superseded by #1680

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality needs testing topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants