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

Added possibility to override context properties in build.yml #213

Merged
merged 4 commits into from
Nov 21, 2021

Conversation

JCash
Copy link
Contributor

@JCash JCash commented Nov 19, 2021

No description provided.

@@ -248,6 +249,15 @@ private PlatformConfig getPlatformConfig(String platform) throws ExtenderExcepti
return platformConfig;
}

private String getPlatformConfigProperty(String property) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allows us to get properties from the PlatformConfig class (read form build.yml)
E.g. "x86_64-win32" will take precedence over the "common" platform.

@@ -216,7 +209,7 @@ public boolean accept(File file) {
if (manifest.platforms.containsKey("common")) {
Object v = manifest.platforms.get("common").context.get(name);
if( v != null ) {
if (!Extender.isListOfStrings((List<Object>) v)) {
if (!ExtenderUtil.isListOfStrings((List<Object>) v)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird circular dependency between this util file and the Extender.java. :/

}

if (v1 != null && v2 != null && v1 instanceof List) {
//System.out.printf("merge op %s : %s + %s (merge: %s)\n", key, v1.toString(), v2.toString(), isMergeOp?"true":"false");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

Comment on lines +354 to +358
if (isMergeOp) {
v1 = ExtenderUtil.mergeLists((List<String>) v1, (List<String>) v2);
} else {
v1 = v2;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the property name ends with "_replace", the else-path here is taken, and we keep the v2 value as-is.

If not, we use the backwards compatible way of merging the two lists v1 and v2.

@@ -11,7 +11,7 @@
public String exePrefix; // deprecated
public String exeExt; // deprecated
public String writeLibPattern;
public String writeShLibPattern = new String();
public String writeShLibPattern;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having it as "null" makes it easier to check for overrides between the platform config instances.

@JCash JCash requested a review from britzl November 19, 2021 16:51
@@ -239,6 +239,7 @@
}

private PlatformConfig getPlatformConfig(String platform) throws ExtenderException {
PlatformConfig commonPlatformConfig = config.platforms.get("common");
Copy link
Contributor

@britzl britzl Nov 19, 2021

Choose a reason for hiding this comment

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

Not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I started out with one idea of "merging" these two classes into one (might do later on), but instead implemented the getPlatformConfigProperty() instead for now.

Copy link
Contributor

@britzl britzl left a comment

Choose a reason for hiding this comment

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

See note on unused variable

@JCash JCash requested a review from britzl November 20, 2021 08:06
Comment on lines +298 to +304
private static final String MERGE_KEY_REPLACE = "_replace";
static private boolean isMergeOp(String name)
{
if (name.endsWith(MERGE_KEY_REPLACE))
return false;
return true;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Q: Do we have a better way than this? Better name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't think of anything right now to be honest.

@JCash JCash merged commit 1b90f75 into dev Nov 21, 2021
@JCash JCash deleted the add-context-override-support branch November 21, 2021 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants