Make it easier to build a working mono-enabled Godot from scons #76542
Conversation
shana
force-pushed
the
shana/csharp-build-improvements
branch
2 times, most recently
from
e12084a to
b451d29
Compare
shana
force-pushed
the
shana/csharp-build-improvements
branch
from
b451d29 to
5588cd0
Compare
There was a problem hiding this comment.
including having a separate Godot installation available to bootstrap the build
Not sure where this comes from. scons build -> gen glue -> build assemblies, should work just fine (except that during the first time build you have to close a message box #75152)
but by default they don't have to do anything, and all binaries and packages are consumed from the bin folder directly. Thoughts?
Passing the --push-nupkgs-local also causes the nuget cache to be cleared of the relevant package version to avoid some caching issues, this should be activated anyways if the new standard build process no longer uses that option.
IMO simplifying the build like this is a good idea, esp if this caches the generated glue and packages.
Did not actually test running this yet; will need updated documentation.
The steps work fine, yes, but they have to be run one by one manually in order to get a complete build. I've also had issues with the exceptions that are thrown on the first run, with godot aborting instead of displaying the popup mentioned on #75152, so I've had to download a usable godot in order to bootstrap the process (Edit: this happens on macos ARM, where it crashes instead of just complaining about the missing runtime) Since these steps are required in order to have a working build, having to lookup/remember a bunch of different command line things is an extra burden that makes things unnecessarily complex for users. If they were optional steps, having them as separate manual steps would make sense, but they're not optional, so incorporating them into the general scons build system makes more sense to me - hence this PR. As someone that does a lot of godot+mono local builds and keeps having to go lookup random commands in the docs (because my memory for these things is bleh), this PR is scratching a personal itch 😆
Ah, good point, yes, the clearing needs to be done even when consuming from the bin/ dir, I'll fix that.
I'll get a PR going for the docs repo. |
shana
force-pushed
the
shana/csharp-build-improvements
branch
2 times, most recently
from
a6f8f4f to
efa3558
Compare
|
Draft PR with updated docs based on these changes is at godotengine/godot-docs#7265 On this one, I've done a bit more rearranging of the |
Updating the docs based on the changes in godotengine/godot#76542
Updating the docs based on the changes in godotengine/godot#76542
There was a problem hiding this comment.
- This still includes the changes from Skip initializing the C# runtime when generating glue bindings #76659 please removed them from this PR. (You can include the other PR as a separate commit if the changes are required, but as far I can tell this PR should work just fine without them)
- Nit: The rebase based workflow godot uses doesn't really like mentioning issues or PRs in the commit message as that ends up spamming the issue with useless links to various commits.
- Looks ok otherwise, but I didn't test the current version so far.
shana
force-pushed
the
shana/csharp-build-improvements
branch
from
efa3558 to
d86b1dd
Compare
Ooops, I'm sorry, I was testing both locally and must have added the native changes by mistake. This PR doesn't need the other one, it was just me doing things really late at night 🙈
Ah, yes, good point. I'll avoid those in the future 👍 |
|
Is this intended for one-time builds rather than for development? Because that's the only scenario I would find this helpful. During development, you're going to want to use 3 separate commands for building the engine, generating the glue and building the assemblies. Otherwise, each step would be much slower than necessary, specially if you're iterating a lot. I'm not sure the extra build logic is worth it for a single use-case, but if enough people want it. Some comments on the current implementation:
|
This isn't meant for one-time builds - any time a user updates their local clone of Godot, these steps might need to run again; any time they change the precision, they'll need to run again. There's a lot of times when these steps need to run, even for users not doing anything special, and it's very easy to forget to run those steps. scons knows how to track changed files and build dependencies, so it only runs the steps if it needs to, there's no extra overhead with having the build system handle these extra build commands. In the majority of cases, it will only run these steps once, and never again, unless you do meaningful changes to either the generated source or to the built artifacts.
I updated the PR description to note that this no longer needs to push nuget packages. Instead, it's configuring a nuget source pointing to the location of the nupkgs in the bin/ folder, so they're always up to date and we skip a copy step entirely, also removing the need for manually configuring a nuget source. It does clear the nuget cache whenever it rebuilds the nuget packages.
If the C# module is enabled but these options aren't, the build will not be functional. I don't think that's a good default UX for normal users. If and when the C# module becomes enabled by default, you/we/someone should probably consider providing users with some sort of pre-configured build profiles, or change the default flags, or something, so that they can make builds out of the box with their preferred language, instead of breaking the C# build by default unless they run additional magic commands. Should also make sure that the C# module is only enabled if the user has the right prerequisites, for example. There's probably a bit of user experience work that should be done as a part of enabling the C# module by default, to make sure that it's not a broken experience out of the box. Given that the C# module is not enabled by default right now, if a user enables it, it's because they want to use it, so they should have a working build in the end, as smoothly as possible. These options are required for that.
We can have one |
|
@neikeq @RedworkDE I was busy moving countries so I couldn't get back to this before now. I've turned the glue generation and assembly build off by default when mono is enabled, and added a |
|
Any updates on this? |
There was a problem hiding this comment.
Also the CI doesn't like the formatting, not an expert for Python or SCons tho, so I can't really comment on the code.
In general neikeq and raulsntos (the dotnet maintainers) currently have no to very limited time to work on godot so C# development is basically paused right now, except for small bug fixes (and even then I have several such PRs that are waiting on reviews for months)
I am not sure how effective this currently is at avoiding unnecessary rebuilds of the dotnet part, tho I don't think it is impossible to make it better than knowing when the glue needs to be rebuilt and (un-) commenting to relevant bits in my build script.
Ah, that is important information, I had no idea, thank you. |
|
I fully support the idea to make it easier to build C# support out of the box 👍 I gave this PR a try locally on Linux, it works well. To be clear in the current iteration it requires passing Regarding @neikeq's concerns on making this enabled by default, I'd like to clarify the use cases we need to cover:
This last use case is what @neikeq is concerned about I believe, and as of the current state of this PR, building with
But for the mono glue generation, this doesn't seem to work full as I would expect it currently. Here's an example:
This means that my incremental build takes 18s instead of the 13s it takes for the same code change without the Mono module. (Generating the Mono glue seems to take ~5s on my system. For comparison, a "null" rebuild with no changes takes 5s, and does not re-generate the mono glue, so that's working fine.) The mono glue generation seems to take the Godot binary as its dependency, so whenever Godot changes (i.e. any non no-op compilation) will trigger a generation of the mono glue. The workflow contributors might currently be used to when working with the Mono module is that they first do the relevant engine changes (which might involve multiple incremental compilations until they get things to compile to what they want), and then do the next steps to test their changes. That being said, 5s extra isn't terrible. Ideally it would only generate the glue when it's really needed, but I'm not sure what the dependencies should be for that. I haven't checked how long the Overall, I'd be in favor of enabling As a side note, fixing the Python style issues would be great so we can get the proper compilation CI going. After installing |
|
Ooops, totally missed your comments here @akien-mga, thanks for taking a look!
Ok, so, yes, I've gone through a couple of iterations on the bindings dependencies, and this version is setting the binary itself as a dependency of the bindings - this is because the list of files that contribute to bindings is not available to scons, it's a database that's built in to the binary itself, and so it is hard to accurately tell scons exactly what should trigger a rebuild. I'm not particularly happy with this, it's too trigger happy. I was experimenting with alternate ways of setting a dependency list, so I'll do some tweaks on this to make it less eager, I think. |
shana
force-pushed
the
shana/csharp-build-improvements
branch
from
b2a7e26 to
e64cb0f
Compare
This adds the `generate_mono_glue=[no,yes]` and `build_assemblies=[no,yes]` flags to scons, which calls the godot binary with `--generate-mono-glue` and the `build_assemblies.py` script, respectively. Having these as part of the build means that building a mono-enabled godot will create all the necessary bits to run it without having to do extra manual steps. It also means that changes to the native code that might change scripting API will automatically trigger regenerating the C# glue bindings and a rebuild of the assemblies, as part of the normal build process. Relevant flags like `precision` and `platform` are passed into the build assemblies step, so the C# libraries match the godot build. Add build_csharp flag to build C# bindings when mono is enabled
shana
force-pushed
the
shana/csharp-build-improvements
branch
from
e64cb0f to
ca866a8
Compare
|
Looks good to me from a very cursory overview, I'll do some more in depth review and testing hopefully tomorrow. |
|
Works fine on my linux!👍 |
|
@akien-mga Ok! So, this new cleanup incorporates all the feedback, except for "if I touch something in an unrelated class, bindings will get regenerated". The command line flags can stop that from happening, but having scons accurately track that automatically is going to be a real pain, as I noted before. If we're ok with that and ok with addressing that later, this should be good to go 😄 |
There was a problem hiding this comment.
So, this new cleanup incorporates all the feedback, except for "if I touch something in an unrelated class, bindings will get regenerated" [...] If we're ok with that and ok with addressing that later, this should be good to go
I think this is important, but as long as the old workflow is still available (the multi-step building) I'm fine with it.
| nupkgs_dir = os.path.abspath("bin/GodotSharp/Tools/nupkgs") | ||
| tools = find_any_msbuild_tool("") | ||
|
|
||
| # configure a default nuget source pointing to the bin/GodotSharp/Tools/nupkgs dir, so packages can be picked up automatically |
There was a problem hiding this comment.
I feel hesitant about changing user configuration by default, I'd rather it be explicitly opt-in.
There was a problem hiding this comment.
I understand, it's a reasonable hesitation. However, without this, C# projects just won't work, which means it is a mandatory step to the whole pipeline - you have to configure a nuget source in order to pick up the packages. Having this opt-in means 99% of users having to run this manually anyways, which defeats the purpose somewhat.
I agree that it's happening too quietly, and with no way to opt-out of it if you have your own custom setup. I don't think it should be opt-in - we should be aiming for the 80/20 rule in user experience (making the 80% most common workflows easier) - but it should be controllable by a flag, so you can disable the behaviour in your own setup and not disrupt thing for very advanced users. I'll add a thing for that - a configure_nuget_source=yes|no flag is probably a thing we want here.
There was a problem hiding this comment.
We've been bitten in the past by making changes to user configuration. In 4.0 we had a GodotNuGetFallbackFolder and changed the user configuration so NuGet would use it. The problem is that, if that folder is removed, then NuGet doesn't work at all. This means we essentially broke all their .NET projects (not just Godot projects), because they could no longer restore packages.
Configuring a NuGet source is a step that only needs to be done once though, so I don't think it's that much to ask. It's the same thing as installing the .NET SDK or setting up other prerequisites before building Godot, and we don't automate those either.
There was a problem hiding this comment.
I've actually experienced this issue last time I tested this PR (and the issue of course happens if following the current manual steps, but it's more explicit to the user that they're adding a local source).
I compiled Godot with Mono support, it generated the local nuget source and everything worked great out of the box 👍
Then some days later I ran git clean -fxd as I often do, it removed the folder from bin, and next time I tried to used an official build of Godot, my .NET setup was broken. I had to ask on chat and be told to check whether my ~/.nuget/NuGet/NuGet.config was referring to something missing.
Is there no way to add a packageSource in that config that wouldn't break dotnet if it's missing? Like with a conditional so it's only added if the folder exists?
There was a problem hiding this comment.
Is there no way to add a
packageSourcein that config that wouldn't break dotnet if it's missing? Like with a conditional so it's only added if the folder exists?
I don't think so, I remember looking at a way to do this in the context of GodotNuGetFallbackFolder but I couldn't find anything.
There was a problem hiding this comment.
Hm :\
I don't know if it's easy to do with scons, but maybe we could make the option output information after the build (the first time the package source needs to be added only) that tells the user about what was done, and how to remove the package source if they need?
I feel like just making the behavior opt-in wouldn't help much because if it's a SCons options, most users won't know what it does and how to undo it anyway.
Another option could be to install the packageSource stuff in the user's NuGet folder (~/.nuget), so that they won't easily delete it by mistake (and I guess it's not too harmful to their other non-Godot stuff, it just has Godot related packages).
There was a problem hiding this comment.
You're absolutely right, I also hit this right after commenting, completely forgot that nuget is the worst. Urgh.
There are three "ways" of avoiding the system nuget failing miserably when a local source path doesn't exist:
-
Nuget supports a
--ignore-failed-sourcesflag on restore to not fail in these situations. You have to pass it in manually, though, so that doesn't help anything, as users (and VS) won't pass this flag in normally. -
Pushing local build packages to a location that is known to always exist: I was trying to avoid pushing packages to another place in the filesystem and instead consume them directly from the bin folder, because that would avoid stale/forgotten local build packages being picked up accidentally, but that's indeed a safe option.
-
Generating nuget.config files: The only way to have nuget sources that don't affect every project is to have a
nuget.configfile next to the csproj, with the local source, so only that project is affected if a source goes away. We could generate this config file in the project folder when a project is opened with a local build of godot-dotnet. Since the csproj in these cases would already referencing a-devversion of the Sdk, it would fail to restore with non-local builds of godot, a failure to restore if the local source path is missing would make it obvious what the failure is, without being confusing to other .net projects.
There was a problem hiding this comment.
I don't think NuGet is the worst, it's just probably not designed for this use case. I think it's weird for a project to add a local source, if you let the user choose the source then it can be whatever they want (it could be a remote source).
Since your intention is to make this "just work", I agree that the first option is not a solution.
The second option is what @akien-mga suggested. I don't really like it, we're still modifying the user's configuration and now we're storing packages somewhere in their computer that they may not be aware of. It feels rude.
I like the third option, but we have to keep in mind that the user may already have a nuget.config file in their project. Not sure how difficult it would be to manage this situation, but it feels like it'd be overkill to implement all this if it's not the most common case.
My opinion is that we're overcomplicating this. Setting up a local source is a step you only have to do once, are there really users complaining about this step or is it a theoretical problem that we're trying to solve here?
| # the only generated files with fixed names are these | ||
| glue_targets = [ |
There was a problem hiding this comment.
Why do we need to explicitly list these files?
There was a problem hiding this comment.
A scons build rule requires a list of sources and a list of targets. This is so that the build system can figure out what build targets need to run when files are changed. In the bindings generation case, we have a problem: we don't exactly know what files contribute to bindings classes, because the binding generation data is calculated while it's running, and there's no metadata information that scons can read that says "this .h/.cpp file is going to generate a .cs file", short of literally regexing every file in the codebase looking for binding information (which, btw, can be done). Therefore we don't have an accurate list of target files to give to scons. We have to give it something, and there are files that are always a part of the code generation, so we can give scons this list. This list doesn't have to be super accurate, because no other build rules rely on this rule for their dependency tracking.
There was a problem hiding this comment.
I see, my concerns are that this list may get outdated so it's one more thing we'll have to keep track of.
There was a problem hiding this comment.
I'm hoping at least the NativeFuncs.cs file will always exist? We only really need one file to give to scons here, so whatever file we are very unlikely to rename will do. It needs to be a file that is generated by the generator, so that if it's missing scons knows it needs to rerun this rule.
We could record the filenames that are generated into a text file, commit that to the repo, and use that as the initial source of files to track, instead of hardcoding a file list here. That way it would be more accurate and wouldn't go stale.
The ideal ideal solution, I think, would be to have a dry-run generator pass that would output the list of .h/.cpp sources and the list of .cs targets relevant to the bindings generator, without doing anything else. We would run that as build step right after the c++ build, and then this rule would have exactly the right list of sources and targets to know when it would need to run.
There was a problem hiding this comment.
Can't we use a globbing pattern here, even if may encompass more files than absolutely necessary?
As a first step we probably want to be conservative and it's better to match more files than needed, than to miss some. And with something like *.cs we don't have to maintain a hardcoded list.
There was a problem hiding this comment.
I think you meant NativeCalls.cs. NativeFuncs.cs is not generated.
I think the file that is least likely to ever be renamed/moved/removed is probably GeneratedIncludes.props, but to be honest I think it's not very likely that any of the files you list here will be.
What you describe as the ideal solution makes sense to me, but maybe we don't need to make it that complicated. Every generated file is always re-generated when generating the glue (even if the generated file would have the same content), so I'm assuming that means SCons will think the file changed. So I don't really see the benefits of this over the current implementation, but maybe I'm missing something.
| from SCons.Variables import BoolVariable | ||
|
|
||
| return [ | ||
| BoolVariable("generate_mono_glue", "Generate C# glue", False), |
There was a problem hiding this comment.
We should move away from the mono naming, it's already confusing to users that the module is still named mono (but it will be renamed to dotnet in the future). I think generate_csharp_glue or generate_dotnet_glue would be better.
There was a problem hiding this comment.
Yes, I agree that we should have renamed it to dotnet from the start, keeping it as mono is all kinds of confusing!
In this case, though, the command line argument for the generator is generate-mono-glue, and this is meant to be an alias to that. Do we want to diverge this flag name? I don't have a super strong opinion either way, really.
There was a problem hiding this comment.
I've added further comments on this below - lookin at all the flags, yeah I agree it would be better to make them more consistent and dotnet-ish.
There was a problem hiding this comment.
I think we should rename the generate-mono-glue argument too. The plan is to rename the module with the Editor unification, at that time we can rename these options too. I was trying to anticipate the rename so we don't have to change the SCons arguments later, but I guess it may be better to be consistent with the current names.
There was a problem hiding this comment.
I see your point, yeah, it would be nice to have the right names now. We can add an alias to the generate-mono-glue argument in preparation for that, and deprecate it later (so it doesn't break everyone's scripts)
There was a problem hiding this comment.
I think we should do the mono -> dotnet rename before 4.2, and thus now-ish. I put it on the agenda for Thursday's meeting.
| return [ | ||
| BoolVariable("generate_mono_glue", "Generate C# glue", False), | ||
| BoolVariable("build_assemblies", "Build C# assemblies", False), | ||
| BoolVariable("build_csharp", "Generate and build C# bindings", False), |
There was a problem hiding this comment.
I think the name build_csharp is confusing with build_assemblies. Maybe we can call this option generate_and_build_csharp or generate_and_build_dotnet.
And, to be honest, build_assemblies being a SCons option, it should probably include csharp or dotnet in the name to be explicit that this option is related to the .NET module.
There was a problem hiding this comment.
The build_csharp flag should be kept short imo, as it is an aggregate flag you use to turn on or off other features and therefore you're likely typing them more often. The other ones can be more verbose, they're going to be used less on average.
If we're tweaking flag names, maybe generate_dotnet and compile_dotnet for the two individual build steps? Maybe with a configure_nuget_source=yes|no option as well, for the nuget step? And then build_dotnet for the aggregate flag, that would make things more consistent.
There was a problem hiding this comment.
Well, generate_and_build_dotnet=yes is still shorter than generate_mono_glue=yes build_assemblies=yes, but I see your point. I don't feel like the verb build implies also generating, but I guess it's a matter of perspective.
To be honest, I don't expect many users to type the SCons commands manually. I've seen users write scripts that execute all the steps.
Building a mono-enabled Godot from source currently requires a bunch of manual steps
, including having a separate Godot installation available to bootstrap the build. The current multi-step bootstrap+build process makes it harder for new users to do their own builds. It would be nice to be able to just doscons target=editor module_mono_enabled=yesand end up with a fully working Godot+Mono build, without extra steps.This PR adds
generate_mono_glueandbuild_assembliesoptions to scons.Having these as part of the build means that building a mono-enabled godot will create all the necessary bits to run it without having to do extra manual steps. It also means that changes to the native code that might change scripting API will automatically trigger regenerating the C# glue bindings and a rebuild of the assemblies, as part of the normal build process. The
build_assemblies.pycan still be run manually from the command line as before.Notes
The one thing this PR doesn't do is pass in the--push-nupkgs-localflag tobuild_assemblies, because that assumes that a dotnet nuget source has already been configured. Which teeeechnically means that game projects will not be fully buildable and users still need to do extra steps for a fully working installation.In this scenario, my personal preference would be for the build to also configure a nuget source pointing to the#bin/GodotSharp/Tools/nupkgsbuild output folder, so the packages get consumed directly from the build and not pushed to somewhere else. Users can then optionally push their nupkgs if they want to distribute them/once they're happy with them, but by default they don't have to do anything, and all binaries and packages are consumed from the bin folder directly. Thoughts?GodotSourceBuildnuget source pointing to#bin/GodotSharp/Tools/nupkgs, so nuget packages built from source are automatically picked up. It also cleans any cached nuget packages.