raphinesse commented on a change in pull request #1212:
URL: https://github.com/apache/cordova-android/pull/1212#discussion_r657956449



##########
File path: bin/lib/create.js
##########
@@ -247,13 +247,16 @@ exports.create = function (project_path, config, options, 
events) {
         ? config.name().replace(/[^\w.]/g, '_') : 'CordovaExample';
 
     var safe_activity_name = config.android_activityName() || 
options.activityName || 'MainActivity';
-    var target_api = check_reqs.get_target();
+    let target_api = parseInt(config.getPreference('android-targetSdkVersion', 
'android'), 10);
+    if (isNaN(target_api)) {
+        target_api = constants.SDK_VERSION;
+    }
 
     // Make the package conform to Java package types
     return exports.validatePackageName(package_name)
         .then(function () {
             return exports.validateProjectName(project_name);
-        }).then(function () {
+        }).then(async function () {

Review comment:
       Reverting leftover `async`
   
   ```suggestion
           }).then(function () {
   ```

##########
File path: bin/lib/create.js
##########
@@ -69,11 +66,14 @@ function copyJsAndLibrary (projectPath, shared, 
projectName, isLegacy) {
     } else {
         fs.ensureDirSync(nestedCordovaLibPath);
         fs.copySync(path.join(ROOT, 'framework', 'AndroidManifest.xml'), 
path.join(nestedCordovaLibPath, 'AndroidManifest.xml'));
-        fs.copySync(path.join(ROOT, 'framework', 'project.properties'), 
path.join(nestedCordovaLibPath, 'project.properties'));
+        const propertiesEditor = createEditor(path.join(ROOT, 'framework', 
'project.properties'));
+        propertiesEditor.set('target', `android-${targetAPI || 
constants.SDK_VERSION}`);
+        propertiesEditor.save(path.join(nestedCordovaLibPath, 
'project.properties'));
         fs.copySync(path.join(ROOT, 'framework', 'build.gradle'), 
path.join(nestedCordovaLibPath, 'build.gradle'));
         fs.copySync(path.join(ROOT, 'framework', 'cordova.gradle'), 
path.join(nestedCordovaLibPath, 'cordova.gradle'));
         fs.copySync(path.join(ROOT, 'framework', 'repositories.gradle'), 
path.join(nestedCordovaLibPath, 'repositories.gradle'));
         fs.copySync(path.join(ROOT, 'framework', 'src'), 
path.join(nestedCordovaLibPath, 'src'));
+        fs.copySync(path.join(ROOT, 'framework', 'defaults.json'), 
path.join(projectPath, 'config.json'));

Review comment:
       Assuming that the file `config.json` is something we invent in this PR, 
I would suggest we try to find a more specific name. From the top of my head: 
maybe something like `native-build-{config,settings,params}.json`?

##########
File path: bin/templates/cordova/lib/prepare.js
##########
@@ -92,6 +76,76 @@ module.exports.prepare = function (cordovaProject, options) {
     });
 };
 
+function updateUserProjectGradleConfig (configXml, defaultGradleConfigPath, 
projectGradleConfigPath) {
+    const defaultGradleConfig = fs.readJSONSync(defaultGradleConfigPath);
+    const profileGradleConfig = fs.readJSONSync(projectGradleConfigPath);
+
+    // Replace modified configs with defaults
+    const mergedConfigs = Object.assign(profileGradleConfig, 
defaultGradleConfig);
+
+    const configXmlToGradleMapping = [
+        { xmlKey: 'android-minSdkVersion', gradleKey: 'MIN_SDK_VERSION', type: 
Number },
+        { xmlKey: 'android-maxSdkVersion', gradleKey: 'MAX_SDK_VERSION', 
default: null, type: Number },
+        { xmlKey: 'android-targetSdkVersion', gradleKey: 'SDK_VERSION', type: 
Number },
+        { xmlKey: 'android-buildToolsVersion', gradleKey: 
'BUILD_TOOLS_VERSION', type: String },
+        { xmlKey: 'GradleVersion', gradleKey: 'GRADLE_VERSION', type: String },
+        { xmlKey: 'AndroidGradlePluginVersion', gradleKey: 'AGP_VERSION', 
type: String },
+        { xmlKey: 'GradlePluginKotlinVersion', gradleKey: 'KOTLIN_VERSION', 
type: String },
+        { xmlKey: 'AndroidXAppCompatVersion', gradleKey: 
'ANDROIDX_APP_COMPAT_VERSION', type: String },
+        { xmlKey: 'GradlePluginGoogleServicesVersion', gradleKey: 
'GRADLE_PLUGIN_GOOGLE_SERVICES_VERSION', type: String },
+        { xmlKey: 'GradlePluginGoogleServicesEnabled', gradleKey: 
'IS_GRADLE_PLUGIN_GOOGLE_SERVICES_ENABLED', type: Boolean },
+        { xmlKey: 'GradlePluginKotlinEnabled', gradleKey: 
'IS_GRADLE_PLUGIN_KOTLIN_ENABLED', type: Boolean }
+    ];
+
+    configXmlToGradleMapping.forEach(mapping => {
+        const configXmlValue = configXml.getPreference(mapping.xmlKey, 
'android');
+
+        if (Object.prototype.hasOwnProperty.call(mapping, 'default')) {

Review comment:
       We never use the value of `mapping.default`

##########
File path: bin/templates/cordova/lib/prepare.js
##########
@@ -92,6 +76,76 @@ module.exports.prepare = function (cordovaProject, options) {
     });
 };
 
+function updateUserProjectGradleConfig (configXml, defaultGradleConfigPath, 
projectGradleConfigPath) {
+    const defaultGradleConfig = fs.readJSONSync(defaultGradleConfigPath);
+    const profileGradleConfig = fs.readJSONSync(projectGradleConfigPath);
+
+    // Replace modified configs with defaults
+    const mergedConfigs = Object.assign(profileGradleConfig, 
defaultGradleConfig);
+
+    const configXmlToGradleMapping = [
+        { xmlKey: 'android-minSdkVersion', gradleKey: 'MIN_SDK_VERSION', type: 
Number },
+        { xmlKey: 'android-maxSdkVersion', gradleKey: 'MAX_SDK_VERSION', 
default: null, type: Number },
+        { xmlKey: 'android-targetSdkVersion', gradleKey: 'SDK_VERSION', type: 
Number },
+        { xmlKey: 'android-buildToolsVersion', gradleKey: 
'BUILD_TOOLS_VERSION', type: String },
+        { xmlKey: 'GradleVersion', gradleKey: 'GRADLE_VERSION', type: String },
+        { xmlKey: 'AndroidGradlePluginVersion', gradleKey: 'AGP_VERSION', 
type: String },
+        { xmlKey: 'GradlePluginKotlinVersion', gradleKey: 'KOTLIN_VERSION', 
type: String },
+        { xmlKey: 'AndroidXAppCompatVersion', gradleKey: 
'ANDROIDX_APP_COMPAT_VERSION', type: String },
+        { xmlKey: 'GradlePluginGoogleServicesVersion', gradleKey: 
'GRADLE_PLUGIN_GOOGLE_SERVICES_VERSION', type: String },
+        { xmlKey: 'GradlePluginGoogleServicesEnabled', gradleKey: 
'IS_GRADLE_PLUGIN_GOOGLE_SERVICES_ENABLED', type: Boolean },
+        { xmlKey: 'GradlePluginKotlinEnabled', gradleKey: 
'IS_GRADLE_PLUGIN_KOTLIN_ENABLED', type: Boolean }
+    ];
+
+    configXmlToGradleMapping.forEach(mapping => {
+        const configXmlValue = configXml.getPreference(mapping.xmlKey, 
'android');
+
+        if (Object.prototype.hasOwnProperty.call(mapping, 'default')) {
+            if (configXmlValue || typeof configXmlValue === 'boolean') {
+                mergedConfigs[mapping.gradleKey] = configXmlValue;
+            } else if (Object.prototype.hasOwnProperty.call(mergedConfigs, 
mapping.gradleKey)) {
+                delete mergedConfigs[mapping.gradleKey];
+            }
+        } else {
+            let value = configXmlValue || 
defaultGradleConfig[mapping.gradleKey];
+
+            switch (mapping.type) {
+            default:
+            case String:
+                value = value.toString();
+                break;
+            case Number:
+                value = parseFloat(value);

Review comment:
       Since we are parsing only SDK versions, this should rather be 
`parseInt(value, 10)`, no?

##########
File path: bin/templates/cordova/lib/prepare.js
##########
@@ -92,6 +76,76 @@ module.exports.prepare = function (cordovaProject, options) {
     });
 };
 
+function updateUserProjectGradleConfig (configXml, defaultGradleConfigPath, 
projectGradleConfigPath) {
+    const defaultGradleConfig = fs.readJSONSync(defaultGradleConfigPath);
+    const profileGradleConfig = fs.readJSONSync(projectGradleConfigPath);
+
+    // Replace modified configs with defaults
+    const mergedConfigs = Object.assign(profileGradleConfig, 
defaultGradleConfig);
+
+    const configXmlToGradleMapping = [
+        { xmlKey: 'android-minSdkVersion', gradleKey: 'MIN_SDK_VERSION', type: 
Number },
+        { xmlKey: 'android-maxSdkVersion', gradleKey: 'MAX_SDK_VERSION', 
default: null, type: Number },
+        { xmlKey: 'android-targetSdkVersion', gradleKey: 'SDK_VERSION', type: 
Number },
+        { xmlKey: 'android-buildToolsVersion', gradleKey: 
'BUILD_TOOLS_VERSION', type: String },
+        { xmlKey: 'GradleVersion', gradleKey: 'GRADLE_VERSION', type: String },
+        { xmlKey: 'AndroidGradlePluginVersion', gradleKey: 'AGP_VERSION', 
type: String },
+        { xmlKey: 'GradlePluginKotlinVersion', gradleKey: 'KOTLIN_VERSION', 
type: String },
+        { xmlKey: 'AndroidXAppCompatVersion', gradleKey: 
'ANDROIDX_APP_COMPAT_VERSION', type: String },
+        { xmlKey: 'GradlePluginGoogleServicesVersion', gradleKey: 
'GRADLE_PLUGIN_GOOGLE_SERVICES_VERSION', type: String },
+        { xmlKey: 'GradlePluginGoogleServicesEnabled', gradleKey: 
'IS_GRADLE_PLUGIN_GOOGLE_SERVICES_ENABLED', type: Boolean },
+        { xmlKey: 'GradlePluginKotlinEnabled', gradleKey: 
'IS_GRADLE_PLUGIN_KOTLIN_ENABLED', type: Boolean }
+    ];
+
+    configXmlToGradleMapping.forEach(mapping => {
+        const configXmlValue = configXml.getPreference(mapping.xmlKey, 
'android');
+
+        if (Object.prototype.hasOwnProperty.call(mapping, 'default')) {
+            if (configXmlValue || typeof configXmlValue === 'boolean') {

Review comment:
       The only mapping entry that has a default key is of type Number, so why 
handle boolean inputs?

##########
File path: framework/cordova.gradle
##########
@@ -151,12 +150,44 @@ def doGetConfigPreference(name, defaultValue) {
     return ret
 }
 
+def doApplyCordovaConfigCustomization() {
+    // Apply user overide properties that comes from the "--gradleArg=-P" 
parameters
+    if (project.hasProperty('cdvMinSdkVersion')) {
+        cordovaConfig.MIN_SDK_VERSION = Integer.parseInt('' + cdvMinSdkVersion)
+    }
+    if (project.hasProperty('cdvSdkVersion')) {
+        cordovaConfig.SDK_VERSION = Integer.parseInt('' + cdvSdkVersion)
+    }
+    if (project.hasProperty('cdvMaxSdkVersion')) {
+        cordovaConfig.MAX_SDK_VERSION = Integer.parseInt('' + cdvMaxSdkVersion)
+    }
+    if (project.hasProperty('cdvBuildToolsVersion')) {
+        cordovaConfig.BUILD_TOOLS_VERSION = cdvBuildToolsVersion
+    }
+    if (project.hasProperty('cdvAndroidXAppCompatVersion')) {
+        cordovaConfig.ANDROIDX_APP_COMPAT_VERSION = cdvAndroidXAppCompatVersion
+    }
+
+    // Ensure the latest installed build tools is selected, with or without 
defined override
+    cordovaConfig.LATEST_INSTALLED_BUILD_TOOLS = 
doFindLatestInstalledBuildTools(
+        cordovaConfig.BUILD_TOOLS_VERSION
+    )
+}
+
 // Properties exported here are visible to all plugins.
 ext {
+    def jsonFile = new File("$rootDir/config.json")
+    def parsedJson = new groovy.json.JsonSlurper().parseText(jsonFile.text)
+
+    cordovaConfig = parsedJson

Review comment:
       Can't we just do the following?
   ```suggestion
       cordovaConfig = new groovy.json.JsonSlurper().parseText(jsonFile.text)
   ```

##########
File path: bin/templates/cordova/lib/prepare.js
##########
@@ -92,6 +76,76 @@ module.exports.prepare = function (cordovaProject, options) {
     });
 };
 
+function updateUserProjectGradleConfig (configXml, defaultGradleConfigPath, 
projectGradleConfigPath) {
+    const defaultGradleConfig = fs.readJSONSync(defaultGradleConfigPath);
+    const profileGradleConfig = fs.readJSONSync(projectGradleConfigPath);

Review comment:
       Should this be `projectGradleConfig` instead of `profileGradleConfig`?

##########
File path: bin/templates/cordova/lib/builders/ProjectBuilder.js
##########
@@ -287,6 +288,16 @@ class ProjectBuilder {
             });
     }
 
+    /**
+     * @private
+     * @returns The user defined configs
+     */
+    _getCordovaConfig () {
+        return JSON.parse(fs.readFileSync(path.resolve(this.root, 
'config.json'), {
+            encoding: 'utf8'
+        }));

Review comment:
       Using fs-extra convenience function:
   ```suggestion
           return fs.readJSONSync(path.join(this.root, 'config.json'));
   ```

##########
File path: bin/templates/cordova/lib/prepare.js
##########
@@ -92,6 +76,76 @@ module.exports.prepare = function (cordovaProject, options) {
     });
 };
 
+function updateUserProjectGradleConfig (configXml, defaultGradleConfigPath, 
projectGradleConfigPath) {
+    const defaultGradleConfig = fs.readJSONSync(defaultGradleConfigPath);
+    const profileGradleConfig = fs.readJSONSync(projectGradleConfigPath);
+
+    // Replace modified configs with defaults
+    const mergedConfigs = Object.assign(profileGradleConfig, 
defaultGradleConfig);

Review comment:
       Is `projectGradleConfig` allowed to define keys that are not in 
`defaultGradleConfig`? If not, then we can omit this and use the defaults 
straight away.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to