Improve internal merging of app/extension manifests #260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # server/src/main/java/com/defold/extender/Extender.java
JCash
commented
Mar 6, 2023
| @@ -88,6 +88,21 @@ class Extender { | |||
| private static final String MANIFEST_ANDROID= "AndroidManifest.xml"; | |||
| private static final String MANIFEST_HTML5 = "engine_template.html"; | |||
|
|
|||
|
|
|||
| // Check that the manifest only contains valid platforms | |||
| private final String[] ALLOWED_MANIFEST_PLATFORMS = new String[] { | |||
Comment on lines
+193
to
+203
| // Merge the platform configs from build.yml into a single instance: common -> platform -> arch-platform | ||
| this.platformConfig = new PlatformConfig(); | ||
| this.platformConfig.context = new HashMap<>(config.context); // the context from build.yml | ||
|
|
||
| for (String platformAlt : ExtenderUtil.getPlatformAlternatives(platform)) { | ||
| PlatformConfig platformConfigAlt = config.platforms.get(platformAlt); | ||
| if (platformConfigAlt == null) | ||
| continue; | ||
|
|
||
| ExtenderUtil.mergeObjects(this.platformConfig, platformConfigAlt); | ||
| } |
There was a problem hiding this comment.
Here's an example where we merge each file (e.g. build.yml) using all the platform names in order:
- common
- win32
- x86_64-win32
Comment on lines
+206
to
+215
| this.platformVariantConfig = new PlatformConfig(); | ||
| if (baseVariantManifest != null) { | ||
| for (String platformAlt : ExtenderUtil.getPlatformAlternatives(platform)) { | ||
| AppManifestPlatformConfig configAlt = baseVariantManifest.platforms.get(platformAlt); | ||
| if (configAlt == null) | ||
| continue; | ||
| PlatformConfig platformConfigAlt = ExtenderUtil.createPlatformConfig(configAlt); | ||
| ExtenderUtil.mergeObjects(this.platformVariantConfig, platformConfigAlt); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same for the variants, e.g. release.appmanifest
Comment on lines
+217
to
+227
| // Merge the app manifest info into a single config | ||
| this.platformAppConfig = new PlatformConfig(); | ||
| for (String platformAlt : ExtenderUtil.getPlatformAlternatives(platform)) { | ||
| AppManifestPlatformConfig configAlt = appManifest.platforms.get(platformAlt); | ||
| if (configAlt == null) { | ||
| continue; | ||
| } | ||
|
|
||
| PlatformConfig platformConfigAlt = ExtenderUtil.createPlatformConfig(configAlt); | ||
| ExtenderUtil.mergeObjects(this.platformAppConfig, platformConfigAlt); | ||
| } |
There was a problem hiding this comment.
And, finally, the game.appmanifest
|
|
||
| resolveVariables(context); |
There was a problem hiding this comment.
Eventually, I think we want to delay as long as possible.
I.e. until the point where it's needed, just before e.g. invoking the command.
Comment on lines
+586
to
+589
| public static PlatformConfig createPlatformConfig(AppManifestPlatformConfig appconfig) throws ExtenderException { | ||
| PlatformConfig config = new PlatformConfig(); | ||
| config.context = mergeMaps(config.context, appconfig.context); | ||
| return config; |
There was a problem hiding this comment.
Since we operate on PlatformConfig, we needed an easy convert function.
Comment on lines
+19
to
+23
| String result = Mustache.compiler().compile(template).execute(context); | ||
| while (!result.equals(template)) { | ||
| template = result; | ||
| result = Mustache.compiler().compile(template).execute(context); | ||
| } |
There was a problem hiding this comment.
A problem we previously had was that if one variable contained another expression, we couldn't resolve that:
env:
a: "foo"
b: "{{env.a}}"
c: "{{env.b}}"
| @@ -28,7 +33,7 @@ public List<String> execute(List<String> templates, Map<String, Object> context) | |||
| List<String> out = new ArrayList<>(); | |||
| for (String template : templates) { | |||
| try { | |||
| out.add(Mustache.compiler().compile(template).execute(context)); | |||
| out.add(this.execute(template, context)); | |||
There was a problem hiding this comment.
Use the internal function that resolves recursively.
| @@ -355,6 +356,60 @@ public void testAppManifestContext() throws IOException, ExtenderException { | |||
| assertEquals( expectedItems, context.get("flags") ); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testMergedContexts() throws IOException, ExtenderException { | |||
There was a problem hiding this comment.
Unit test for my actual use case (removing profiler libraries)
Comment on lines
+215
to
+218
| if (!this.cacheClearOnExit) { | ||
| LOGGER.info("Skipping cleanup of SDK cache"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
By not deleting the cache, the local tests should go faster, by not having to download the sdk's all the time.
britzl
approved these changes
Mar 8, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem was that the merging/overrides was done in an unintuitive manner.
Now, most of the merging code was moved to the
Extender.loadManifests()function which is called at the start of the job.The issue at hand was that we needed to override (and exclude) some engine libraries (for the profiler), but the previous way of merging happened too late.
At later stages, the build steps may use the extension or app context to replace environment variables accordingly.