-
-
Notifications
You must be signed in to change notification settings - Fork 587
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
Add scons boilerplate for building library and test on Android/iOS, add them to CI. #635
Conversation
Add it to CI.
os: ubuntu-18.04 | ||
platform: linux | ||
artifact-name: godot-cpp-linux-glibc2.27-x86_64-release | ||
artifact-path: bin/libgodot-cpp.linux.release.64.a |
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.
Do these artifact names and paths make sense? it's not the libgodot-cpp that forms the artifact of the plugin itself.
We probably need to store the name of the library and its path somewhere at the top in a variable and then reuse it to build the final artifact name so it's easy to update the CI for your own library
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.
Actually thinking about this, we'll also need to add something to build the deliverable. I still want to do this for the godot-cpp CI as well, where we gather all header files and all compiled libraries and combine them in a downloadable library zip.
Have a look at https://github.com/GodotVR/godot_openxr/blob/master/.github/workflows/build-on-push.yml#L143 for an example of how we currently do this for OpenXR
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.
And I suddenly realise the top remark is DUMB because offcourse we build both the godot-cpp and the test library in CI here :( Sorry I just woke up
opts.Add(EnumVariable("target", "Compilation target", "debug", allowed_values=("debug", "release"), ignorecase=2)) | ||
opts.Add( | ||
PathVariable("target_path", "The path where the lib is installed.", default_target_path, PathVariable.PathAccept) | ||
) | ||
opts.Add(PathVariable("target_name", "The library name.", default_library_name, PathVariable.PathAccept)) |
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.
Worth a discussion whether we actually want target_name as a path variable, I have yet to find a good use case where you would change this :) I think it was added way back when we first started with GDNative more out of habit than for any real reason.
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'm sure we'll find the need for tweaks and I think there are some improvements we can do on the CI (regardless of my dumb remark because I was too sleepy), but this looks good enough to merge to me
In this PR:
Note
A lot of the boilerplate in the test is a copy of the one in the
SConstruct
of the library itself.I was wondering, given the way scons works, that maybe we might want to instead the users to use the library
SConstruct
, adding abuild_projects=path/to/proj1,../path/to/proj2
which will automatically rebuild the library (if needed) and then include a minimalSConstruct
with the env already setup (and where they only add custom flags, sources, etc). cc @BastiaanOlij