-
-
Notifications
You must be signed in to change notification settings - Fork 76
Add support for experimental .NET builds (GDExtension) #136
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
Conversation
|
|
||
| git clone https://github.com/raulsntos/godot-dotnet | ||
| cd godot-dotnet | ||
| git checkout upgrade-assistant-plus-source-code-plugin-wip |
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.
Currently it's using the latest commit on the WIP branch.
We'll have to decide whether we want to have a local Git checkout and set the commit manually (like we do for Godot itself), or always build the latest dev branch. I'm not sure how the versioning for godot-dotnet will be like long term.
build-dotnet/build.sh
Outdated
| cd godot-dotnet | ||
| git checkout upgrade-assistant-plus-source-code-plugin-wip | ||
|
|
||
| ./build.sh --productBuild --warnAsError false /p:GenerateGodotBindings=true |
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.
Should we build Godot to generate the API json and before building godot-dotnet?
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.
There is a small chance it could break godot-dotnet, so we probably don't want to do this in the script when building a release, we should do it before building a release. We'd push the changes to the repo like godot-cpp (example: godotengine/godot-cpp@e83fd09).
The only difference is godot-cpp makes those commits after a release, and we probably want to do it before the release but after we tagged the commit we'll use for the release. So that we can publish the NuGet packages at the same time.
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.
We might want to add some logic to publish-release.sh to do that sync in a local copy of godot-dotnet and push it then, so it's automated.
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.
What I'm saying is that sometimes the required changes can't be automated. For example:
dev3added new API to GDExtension with a pointer type we don't recognize so I added code to exclude the API for now until we can properly support the type: raulsntos/godot-dotnet@8d84f62dev4added a new fieldhash_compatibilitythat we weren't expecting, we intentionally want to fail on unknown fields so it can notify us when new fields are added. So I added the code to recognize the field: raulsntos/godot-dotnet@4e9d54d
build-release.sh
Outdated
| can_sign_windows=0 | ||
| if [ ! -z "${WINDOWS_SIGN_NAME}" ] && [ ! -z "${WINDOWS_SIGN_URL}" ] && [[ $(type -P "osslsigncode") ]]; then | ||
| can_sign_windows=1 | ||
| can_sign_windows=0 |
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.
Just a hack because Windows signing currently requires a manual step to unlock the key, so for my test builds I don't bother signing them.
| elif [[ "{$templates_version}" == *"-"* ]]; then | ||
| echo "Templates version (-t) shouldn't contain '-'. It should use a dot to separate version from status." | ||
| exit 1 |
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.
Another hack because the logic isn't clever enough to deal with 4.6.dev-dotnet-a4e0a369d, though it's valid for Godot.
build-release.sh
Outdated
| dotnetname="godot-dotnet-${templates_version}" | ||
| mkdir ${dotnetname} | ||
| cp out/dotnet/* ${dotnetname}/ | ||
| zip -q -9 -r "${reldir}/${dotnetname}.zip" ${dotnetname} | ||
| rm -rf ${dotnetname} |
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.
Any further steps we can take to make this easier to use?
E.g. provide a Python script that configures a local Nuget source and installs those packages.
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.
We can provide a script, but whatever directory we use can't be removed or it could break restoring packages in any C# project (including non-Godot projects). Also, users would probably want to remove the local NuGet source after the previews and I expect them to forget.
So I think automating this with a script may cause more trouble than the problems it fixes, I'm getting flashbacks to GodotNuGetFallbackFolder.
build-web/build.sh
Outdated
| "target=template_debug dlink_enabled=yes module_dotnet_enabled=yes" | ||
| "target=template_release dlink_enabled=yes module_dotnet_enabled=yes" |
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 don't expect Web to work just yet, but if/when it does, I assume it would only work for the dlink_enabled builds (i.e. GDExtension support).
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, the WIP PR I made requires the opposite. It didn't work with dlink_enabled because too many symbols or something like that. But we don't need dynamic linking because Mono is linked statically.
We may need the statically-linked Mono template to be separate if we don't want that to be included in GDScript-only projects.
| $SCONS platform=android arch=x86_64 $OPTIONS module_dotnet_enabled=yes target=template_debug | ||
|
|
||
| pushd platform/android/java | ||
| ./gradlew generateGodotTemplates |
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.
This is the wrong gradle target, we have to use generateGodotMonoTemplates so it includes the .NET .jar dependencies.
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.
For some reason I can't mark comments as resolved, but this has been fixed in the last commit.
build-dotnet/build.sh
Outdated
| cd godot-dotnet | ||
| git checkout upgrade-assistant-plus-source-code-plugin-wip | ||
|
|
||
| ./build.sh --productBuild --warnAsError false /p:GenerateGodotBindings=true |
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.
For official releases we want to build with the following options:
./build.sh --productBuild --ci --warnAsError false \
/p:GenerateGodotBindings=true \
/p:VersionPrefix=${version_prefix} \
/p:OfficialBuildId=${build_id} \
/p:FinalVersionKind=${final_version_kind} \
/p:PreReleaseVersionLabel=${prerelease_label}The --ci and /p:OfficialBuildId options are for making deterministic builds. The /p:VersionSuffix, /p:FinalVersionKind, and /p:PreReleaseVersionLabel options are to override the version of the packages with whatever is passed to the scripts so we match the editor version. (raulsntos@ca22bd4#diff-7ea7ad2452885a8abbd9bc73beed16c3f9cf613f94049bf94b61746c7fc80f1aR30-R74)
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.
This has been resolved as well.
|
|
||
| # Config | ||
|
|
||
| dnf install -y clang python-unversioned-command |
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 didn't need to install python-unversioned-command in my changes, is this to fix this error?:
urllib.request.urlretrieve fails with <urlopen error unknown url type: https>
I "fixed" it by downloading the .NET SDK outside of the container. (raulsntos@ca22bd4#diff-4d2a8eefdf2a9783512a35da4dc7676a66404b6f3826a8af9aad038722da6823R273-R285)
But we could alternatively download the .NET SDK with the dotnet install script in the container image, so it's pre-downloaded, and then in this build script we just copy it to the .dotnet directory. We just need to make sure it matches the version required in global.json (which right now is a moving target).
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 didn't get that error myself. It's a bit weird that you fixed it by installing .NET outside the container, as it shouldn't use anything outside the container when in-container.
The reason I needed python-unversioned-command is that there's no /usr/bin/python by default in a barebones Fedora image. You had it working because you used our Linux SDK which provides python, but I don't think it's relevant to use our Linux SDK here as we don't need to create portable Linux binaries with GCC (I assume we only use dotnet and ClangSharp in this build?).
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.
It's a bit weird that you fixed it by installing .NET outside the container, as it shouldn't use anything outside the container when in-container.
To be clear, when I said downloading .NET SDK outside the container, this just creates a .dotnet directory inside the cloned repo that gets archived and sent to the container.
I don't think it's relevant to use our Linux SDK here as we don't need to create portable Linux binaries with GCC (I assume we only use dotnet and ClangSharp in this build?)
That's correct, I just copied it from the build-mono-glue script without giving it much thought.
fdcf770 to
b6bfcb1
Compare
|
I reworked this to add a new "dotnet" flavor like you did in your commit, and added relevant changes to |
raulsntos
left a comment
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.
The changes look good to me. I haven't tested the scripts, but I tested the Linux dev4 build.
| export SCONS="scons -j$(expr ${NUM_CORES} / ${NUM_JOBS}) verbose=yes warnings=no progress=no redirect_build_objects=no" | ||
| export OPTIONS="production=yes" | ||
| export OPTIONS_MONO="module_mono_enabled=yes -j${NUM_CORES}" | ||
| export OPTIONS_MONO="module_dotnet_enabled=yes -j${NUM_CORES}" |
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.
| export OPTIONS_MONO="module_dotnet_enabled=yes -j${NUM_CORES}" | |
| export OPTIONS_DOTNET="module_dotnet_enabled=yes -j${NUM_CORES}" |
There's a typo here, although it's not too important since we don't support web yet.
Also, it's interesting that the web script is the only one using -j in the mono/dotnet options, is that intentional?
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.
Yeah that's because we use a weird hack for the classical build where we build with $NUM_CORES / 5 and start 4 builds in parallel. This is because what takes the most time is linking the binaries with LTO.
For Mono we restore the full -j $NUM_CORES and build the two templates sequentially. Not my proudest code :P
This is eventually meant to be unified with the "classical" builds, but for the initial testing we start with a dedicated "dotnet" build. Co-authored-by: Raul Santos <raulsntos@gmail.com>
b6bfcb1 to
0cc7d34
Compare
|
This was tested successfully with 4.6-dev5 to confirm it does not break making Classical + Mono builds only (default). |
Currently hacked on top of classical build, should be split into a separate config as initially we'll still ship separate builds with the .NET module for testing.This is eventually meant to be unified with the "classical" builds, but for the initial testing we start with a dedicated "dotnet" build.
Updated with some code from raulsntos@ca22bd4