-
-
Notifications
You must be signed in to change notification settings - Fork 86
Misc fixes and cleanup #289
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
804514b to
a586acc
Compare
|
I've actually been working on something similar here (a bit messed up now since it all started while trying to get windows arm builds). EDIT: I'll give some feedback tomorrow, I think overall your changes are good, but we should probably cleanup a bit more. |
|
Your changes look pretty good, definitely worth going in that direction. The I'm happy to undo some of the changes in this PR to make it more minimal and avoid conflicts with the more comprehensive overhaul you're doing (like the couple removals I made in My goal for this PR is just to do a small cleanup before we tag a release with latest OpenSSL for all platforms. Then we can merge mbedTLS and further refactoring work for the following release. |
.github/workflows/build.yml
Outdated
| - name: Prepare artifact | ||
| run: | | ||
| mkdir bin | ||
| mv addons bin/ |
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 had to add this awkward step because after removing the demo/ nesting, I ran into an issue with the artifact, where the zip wouldn't contain the addons/ top-level folder.
upload-artifact@v4 doesn't give any way to configure how to zip the given path sadly, so I need to trick it by re-introducing a parent folder to zip the contents of...
Another alternative I considered is to bring back a top-level folder to contain addons (like bin/addons/godot-git-plugin/), but I find it a bit annoying.
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.
Well s/bin/out/ because apparently we do have some thirdparty stuff that generates in bin.
|
I tried to download a zip of the repo and noticed we have a Should probably be removed, we don't store binaries in the repo so there's not much point to try to make a "ready-to-use" zip archive from git. |
5689692 to
637785b
Compare
|
I tested |
|
FYI, for releases, I've adapted (and actually improved) the CI pipeline I'm using for webrtc-native, which uses the download-artifacts actions and produces a packaged See the pipeline here: https://github.com/Faless/godot-git-plugin/actions/runs/15298595568 |
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 think this is a good starting point, and okay for the OpenSSL release.
I suggest that when we update to mbedTLS we further refactor the build system moving the src folder to the top level.
I would also recommend we start using a pipeline that makes all the builds and package them, making doing releases is much easier (see the above comment, and #291).
|
I noticed the [configuration]
entry_symbol = "git_plugin_init"
compatibility_minimum = "4.2.0"
[libraries]
linux.editor.x86_64 = "linux/libgit_plugin.linux.editor.x86_64.so"
linux.editor.arm64 = "linux/libgit_plugin.linux.editor.arm64.so"
macos.editor = "macos/libgit_plugin.macos.editor.universal.dylib"
windows.editor.x86_64 = "win64/libgit_plugin.windows.editor.x86_64.dll"I would probably remove that reference until we add ARM64 to it. PS: Do we need to include the |
Done.
Yeah I don't think those are needed, I pushed an update to remove them. I also renamed the target folder for Windows libs to |
Calinou
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.
Looks good to me.
- Remove the `demo` project which isn't really useful. * Instead the `addons` subfolder is made top-level and that's where stuff gets written. - Update the README following recent PRs (godotengine#191, godotengine#199) as OpenSSL is now compiled from source for all platforms. * Also remove obsolete `build_openssl_universal_macos.sh` script. - Remove the bogus `plugin.cfg` which isn't necessary, and was confusing users who tried to enable it. * Fixes godotengine#267. - Bump min SCons version to 3.1.2 and Python to 3.6. - Bump `compatibility_minimum` to `4.2.0` following godotengine#196. - Remove some code in `godot-git-plugin/SCsub` that seems redundant with `godot-cpp` config. - Remove unnecessary `.exp` and `.lib` files in Windows artifact, rename its folder to `windows`. - Remove `export-ignore`s in `.gitattributes`, they're incomplete and not actually doing anything usable. - Fix artifacts URL handling in `release.sh`, make it executable. - CI: Update clang-format check to version 18.
demoproject which isn't really useful (had a scene and a script not attached to the scene, and it can't really be used to test committing stuff as that would conflict with the plugin's config).addonssubfolder is made top-level and that's where stuff gets written.demo.build_openssl_universal_macos.shscript.plugin.cfgwhich isn't necessary, and was confusing users who tried to enable it.4.3- older versions of SCons don't really work with recent Python anymore).compatibility_minimumto4.2.0following Update plugin for Godot 4.2 due to GDExtension compatibility breakage #196.godot-git-plugin/SCsubthat seems redundant withgodot-cppconfig..expand.libfiles in Windows artifact, rename its folder towindows.export-ignores in.gitattributes, they're incomplete and not actually doing anything usable.release.sh, make it executable.