firebelley / godot-export

Automatically exports your Godot games.
MIT License
460 stars 53 forks source link

Output + Build Improvements + Various Features #116

Open moorbren opened 1 year ago

moorbren commented 1 year ago

Added a number of fixes, features and tweaks:

Let me know your thoughts, thanks.

firebelley commented 11 months ago

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.

moorbren commented 11 months ago

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}`,
      );
    }
  }

Fixed bug with self-hosted runners where new archives wouldn't overwrite old ones (this one may have been fixed with https://github.com/firebelley/godot-export/issues/118)

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.

firebelley commented 11 months ago

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!

RedGlow commented 9 months ago

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?

moorbren commented 9 months ago

@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.

moorbren commented 9 months ago

@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.

cmr624 commented 5 months ago

@firebelley this feature to choose specific presets would be really helpful! could we merge this PR? cc @moorbren

moorbren commented 5 months ago

@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