Skip to content
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

Fix the gradle build configuration for the Android platform #66935

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Oct 5, 2022

Refactor needed following #66242

@m4gr3d m4gr3d added this to the 4.0 milestone Oct 5, 2022
@m4gr3d m4gr3d requested a review from a team as a code owner October 5, 2022 15:27
@m4gr3d m4gr3d changed the title Fix the gradle build configuration for the Android platform [WIP] Fix the gradle build configuration for the Android platform Oct 5, 2022
@m4gr3d m4gr3d force-pushed the fix_android_build_configuration branch from 0a1b686 to 3178b04 Compare October 5, 2022 15:42
@m4gr3d m4gr3d changed the title [WIP] Fix the gradle build configuration for the Android platform Fix the gradle build configuration for the Android platform Oct 5, 2022
Comment on lines 32 to +38
supportedFlavors = ["editor", "template"]
supportedFlavorsBuildTypes = [
// The editor can't be used with target=release as debugging tools are then not
// included, and it would crash on errors instead of reporting them.
"editor": ["dev", "debug"],
"template": ["dev", "debug", "release"]
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what gradle needs but I think it would be clearer if we followed the same structure as in scons, so:

supportedFlavors = ["editor", "template_debug", "template_release"]
supportedBuildTypes = ["regular", "dev"]  // In SCons it's just a boolean, so could be the same here if that works.

dev_build=yes can now be used with any target so target=template_release dev_build=yes is also a valid use case (gives template_release.dev lib). It's not a particularly useful combination but it's IMO better if gradle follows the same logic of having only three targets and then a bool for dev_build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new build model unfortunately doesn't match with Gradle's build layout.
Gradle has the concept of build types and product flavors:

  • A product flavor is a different version of the project (e.g: demo version vs full version). This aligned somewhat better with the previous build architecture where tools=no corresponded to the template flavor and tools=yes corresponded to the editor flavor
  • A build type on the other hand mapped to our previous concept of target and is used configure the build configuration (debug symbols, optimization, etc)

With the new build model, we still have two product flavors ('editor' and 'template') and still three build types ('dev', 'release_debug' and 'release'), so the gradle build model is still valid, we just need to update the mapping when we generate the scons command.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build types are also shared across the project modules, while the flavors are linked:

  • The app module maps to the template flavor
  • The editor module maps to the editor flavor
  • As a dependency, the lib module is configured to automatically update its flavor and build type to match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great, thanks for the explanation :)

@m4gr3d m4gr3d merged commit 8afa178 into godotengine:master Oct 5, 2022
@m4gr3d m4gr3d deleted the fix_android_build_configuration branch October 5, 2022 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants