-
Notifications
You must be signed in to change notification settings - Fork 66
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
Output + Build Improvements + Various Features #116
Conversation
- index is now minified, half the size and less likely to appear in search - added shx to allow cross platform rm with windows
- each build is very noisy. easier to find issues with individual platform exports
- By default we export everything
- If it's not, the Android build fails in an incredibly cryptic way
- Switched to environment vars instead of editor config, much cleaner as well
- Speeds up self-hosted builds significantly
- Fixes a bug with self hosted runners where archives are out of date since 7zip won't overwrite files
Hello, thanks for opening this. I'm not super comfortable with the android changes - imo any android setup should be done as a separate step in a workflow prior to running this action. Otherwise this action is becoming too bloated (ideally it's just downloading Godot + export templates and doing the export commands). These changes are certainly welcome:
If you'd like to open a new PR with just these changes I would accept that and get it merged. But I am open to your thoughts on the Android features. |
I can pull out the keystore setting stuff, since it's not really needed in here. I wrote that whole fea;ture before I realized that you can override a lot of the project level settings via environment variables (which users can do themselves super easily). However, the Android SDK setting is an editor level setting which are pretty tricky for user's to modify themselves since GodotExport creates the editor config directly. Instead, I could convert this to a more generic 'custom_export_settings' input so people can add Editor settings themselves. However, the issue with this approach is resolving duplicate custom editor settings. Where GodotExport sets a default editor setting (ex: /usr/local/lib/android/sdk) and a user adds a custom setting, reconciling would be a total pain. I'm a fan of the more direct approach here which is just adding an input variable for some of the more common editor variables that need to be added. I would like to keep the gradlew one in though since that was a huge gotcha for me. I develop on Windows and had no issues exporting the Android build locally, but it kept crashing cryptically at the Android stage. It's a pretty small add for a pretty common issue people will run into when moving from local builds to cloud Linux builds. Here's the full code for the gradlew fix under configureAndroidExport around L426 in godot.ts: if (process.platform !== 'win32') {
try {
if (fs.existsSync(path.join(GODOT_PROJECT_PATH, 'android/build/gradlew'))) {
fs.chmodSync(path.join(GODOT_PROJECT_PATH, 'android/build/gradlew'), '755');
}
core.info('Made gradlew executable.');
} catch (error) {
core.warning(
`Could not make gradlew executable. If you are getting cryptic build errors with your Android export, this may be the cause. ${error}`,
);
}
}
Different issue than that, the one I'm referring to is an issue with the 7zip command in the 'zipBuildResult' function. Previously, it was checking if the file existed before export and wouldn't zip if the file existed. Just removed the exists check as the 'a' option overwrites existing zip files automatically. else if (!fs.existsSync(zipPath)) {
await exec('7z', ['a', zipPath, `${buildResult.directory}${ARCHIVE_ROOT_FOLDER ? '' : '/*'}`]);
} LMK what you think, appreciate the thoughts on this. |
For editor level settings, there is a section of the docs that specifies how to supply your own editor settings file: https://github.com/firebelley/godot-export#supplying-a-custom-editor-settings-file I was thinking this could be leveraged to do most of the android configuration that you've mentioned as a prior step in a users workflow. If you agree, documentation about any android-level config settings would be great to have. Is the gradle setup just a permission change to the executable? If so, then yeah I think that's perfectly fine to leave in! |
A question: would the "Ability to specify which presets you want to export, defaults to all if none specified" allow for concurrent builds using a matrix strategy? |
@RedGlow No it's still linear at the moment. I'll clean this up and have it ready to merge in, feel free to play around with concurrent builds. I'm not sure how well it'll work though, the build for Godot is pretty jank and having two builds running at the same time may break things. |
- if there is an error in the script, it makes it very difficult to hunt down the source of the problem - thus we leave it nicely readable for humans
@firebelley Should be ready to merge in now. The android stuff has been reverted minus the Gradle executible patch. Let me know if something else needs to be changed prior to a merge. |
@firebelley this feature to choose specific presets would be really helpful! could we merge this PR? cc @moorbren |
@firebelley Yeah this has been ready to merge for a while. Feel free to make me a maintainer if you have too much on your plate atm. I'm cool if you want to revoke it later down the line |
@firebelley Any update on this? Just want to get this resolved. |
@moorbren hello, sorry for the long wait. I'm going to set aside some time to look at this + other issues soon. |
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 great! Sorry for the delay. I may make some minor tweaks before releasing, but expect a new release later today.
Added a number of fixes, features and tweaks:
core.getBooleanInput
instead now. Allows saying something like TRUE or True as well (same for false)Let me know your thoughts, thanks.