You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@cordova.apache.org by GitBox <gi...@apache.org> on 2021/04/18 12:00:32 UTC
[GitHub] [cordova-android] raphinesse commented on a change in pull request #1211: enhancement: Control project properties in one place
raphinesse commented on a change in pull request #1211:
URL: https://github.com/apache/cordova-android/pull/1211#discussion_r615382099
##########
File path: bin/templates/cordova/lib/builders/ProjectBuilder.js
##########
@@ -265,10 +265,11 @@ class ProjectBuilder {
}).then(function () {
return self.prepBuildFiles();
}).then(() => {
+ const defaults = this._getCordovaDefaults();
Review comment:
```suggestion
const { DEFAULT_GRADLE_VERSION } = this._getCordovaDefaults();
```
##########
File path: framework/cordova.gradle
##########
@@ -155,8 +170,9 @@ def doGetConfigPreference(name, defaultValue) {
ext {
// These helpers are shared, but are not guaranteed to be stable / unchanged.
privateHelpers = {}
+ privateHelpers.getCordovaDefaults = { doGetCordovaDefaults() }
privateHelpers.getProjectTarget = { doGetProjectTarget() }
- privateHelpers.findLatestInstalledBuildTools = { doFindLatestInstalledBuildTools('19.1.0') }
+ privateHelpers.findLatestInstalledBuildTools = { doFindLatestInstalledBuildTools("29.0.2") }
Review comment:
Is that version not something we can/should move to a config? I'm seriously asking, no idea.
##########
File path: bin/templates/cordova/lib/check_reqs.js
##########
@@ -42,36 +42,19 @@ Object.assign(module.exports, { isWindows, isDarwin });
*/
module.exports.get_target = function () {
const projectPropertiesPaths = [
- path.join(REPO_ROOT, 'framework', 'project.properties'),
+ // path.join(REPO_ROOT, 'framework', 'project.properties'),
path.join(PROJECT_ROOT, 'project.properties')
];
// Get the minimum required target API from the framework.
- let target = projectPropertiesPaths.filter(filePath => fs.existsSync(filePath))
+ const target = projectPropertiesPaths.filter(filePath => fs.existsSync(filePath))
.map(filePath => createEditor(filePath).get('target'))
.pop();
if (!target) {
throw new Error(`We could not locate the target from the "project.properties" at either "${projectPropertiesPaths.join('", "')}".`);
}
- // If the repo config.xml file exists, find the desired targetSdkVersion.
Review comment:
Did you remove a feature here or just change its implementation? Or is this still WIP, seeing the commented-out bits above?
It looks like the removed part was moved to `create.js`?
##########
File path: bin/templates/cordova/lib/builders/ProjectBuilder.js
##########
@@ -287,6 +288,16 @@ class ProjectBuilder {
});
}
+ /**
+ * @private
+ * @returns The defaults file contents
+ */
+ _getCordovaDefaults () {
+ return JSON.parse(fs.readFileSync(path.resolve(this.root, 'defaults.json'), {
+ encoding: 'utf8'
+ }));
Review comment:
`fs-extra` provides helpers for reading & parsing JSON from file:
```suggestion
return fs.readJsonSync(path.resolve(this.root, 'defaults.json'));
```
##########
File path: spec/unit/check_reqs.spec.js
##########
@@ -294,62 +291,63 @@ describe('check_reqs', function () {
}).toThrow();
});
- it('should override target from config.xml preference', () => {
- var realExistsSync = fs.existsSync;
- spyOn(fs, 'existsSync').and.callFake(function (path) {
- if (path.indexOf('config.xml') > -1) {
- return true;
- } else {
- return realExistsSync.call(fs, path);
- }
- });
-
- getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API + 1);
-
- var target = check_reqs.get_target();
-
- expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
- expect(target).toBe('android-' + (DEFAULT_TARGET_API + 1));
- });
-
- it('should fallback to default target if config.xml has invalid preference', () => {
- var realExistsSync = fs.existsSync;
- spyOn(fs, 'existsSync').and.callFake(function (path) {
- if (path.indexOf('config.xml') > -1) {
- return true;
- } else {
- return realExistsSync.call(fs, path);
- }
- });
-
- getPreferenceSpy.and.returnValue(NaN);
-
- var target = check_reqs.get_target();
-
- expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
- expect(target).toBe('android-' + DEFAULT_TARGET_API);
- });
-
- it('should warn if target sdk preference is lower than the minimum required target SDK', () => {
- var realExistsSync = fs.existsSync;
- spyOn(fs, 'existsSync').and.callFake(function (path) {
- if (path.indexOf('config.xml') > -1) {
- return true;
- } else {
- return realExistsSync.call(fs, path);
- }
- });
-
- spyOn(events, 'emit');
-
- getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API - 1);
-
- var target = check_reqs.get_target();
-
- expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
- expect(target).toBe('android-' + DEFAULT_TARGET_API);
- expect(events.emit).toHaveBeenCalledWith('warn', 'android-targetSdkVersion should be greater than or equal to ' + DEFAULT_TARGET_API + '.');
- });
+ // TODO(Breautek) Delete these tests if they are not actually required...
+ // it('should override target from config.xml preference', () => {
+ // var realExistsSync = fs.existsSync;
+ // spyOn(fs, 'existsSync').and.callFake(function (path) {
+ // if (path.indexOf('config.xml') > -1) {
+ // return true;
+ // } else {
+ // return realExistsSync.call(fs, path);
+ // }
+ // });
+
+ // getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API + 1);
+
+ // var target = check_reqs.get_target();
+
+ // expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
+ // expect(target).toBe('android-' + (DEFAULT_TARGET_API + 1));
+ // });
+
+ // it('should fallback to default target if config.xml has invalid preference', () => {
+ // var realExistsSync = fs.existsSync;
+ // spyOn(fs, 'existsSync').and.callFake(function (path) {
+ // if (path.indexOf('config.xml') > -1) {
+ // return true;
+ // } else {
+ // return realExistsSync.call(fs, path);
+ // }
+ // });
+
+ // getPreferenceSpy.and.returnValue(NaN);
+
+ // var target = check_reqs.get_target();
+
+ // expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
+ // expect(target).toBe('android-' + DEFAULT_TARGET_API);
+ // });
+
+ // it('should warn if target sdk preference is lower than the minimum required target SDK', () => {
+ // var realExistsSync = fs.existsSync;
+ // spyOn(fs, 'existsSync').and.callFake(function (path) {
+ // if (path.indexOf('config.xml') > -1) {
+ // return true;
+ // } else {
+ // return realExistsSync.call(fs, path);
+ // }
+ // });
+
+ // spyOn(events, 'emit');
+
+ // getPreferenceSpy.and.returnValue(DEFAULT_TARGET_API - 1);
+
+ // var target = check_reqs.get_target();
+
+ // expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
+ // expect(target).toBe('android-' + DEFAULT_TARGET_API);
+ // expect(events.emit).toHaveBeenCalledWith('warn', 'android-targetSdkVersion should be greater than or equal to ' + DEFAULT_TARGET_API + '.');
+ // });
Review comment:
We should figure out if we need those or not before merging.
##########
File path: bin/templates/cordova/lib/builders/ProjectBuilder.js
##########
@@ -265,10 +265,11 @@ class ProjectBuilder {
}).then(function () {
return self.prepBuildFiles();
}).then(() => {
+ const defaults = this._getCordovaDefaults();
// update/set the distributionUrl in the gradle-wrapper.properties
const gradleWrapperPropertiesPath = path.join(self.root, 'gradle/wrapper/gradle-wrapper.properties');
const gradleWrapperProperties = createEditor(gradleWrapperPropertiesPath);
- const distributionUrl = process.env.CORDOVA_ANDROID_GRADLE_DISTRIBUTION_URL || 'https://services.gradle.org/distributions/gradle-6.8.3-all.zip';
+ const distributionUrl = process.env.CORDOVA_ANDROID_GRADLE_DISTRIBUTION_URL || `https://services.gradle.org/distributions/gradle-${defaults.DEFAULT_GRADLE_VERSION}-all.zip`;
Review comment:
```suggestion
const distributionUrl = process.env.CORDOVA_ANDROID_GRADLE_DISTRIBUTION_URL || `https://services.gradle.org/distributions/gradle-${DEFAULT_GRADLE_VERSION}-all.zip`;
```
##########
File path: framework/gradle/wrapper/gradle-wrapper.properties
##########
@@ -3,4 +3,3 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-4.6-all.zip
Review comment:
Sure we do not need this anymore? I seriously do not know, just want to bring it to attention.
##########
File path: bin/lib/create.js
##########
@@ -69,11 +66,19 @@ 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'));
- 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'));
+ TemplateFile.render(path.join(ROOT, 'framework', 'project.properties'), path.join(nestedCordovaLibPath, 'project.properties'), {
+ DEFAULT_SDK_VERSION: targetAPI || constants.DEFAULT_SDK_VERSION,
+ DEFAULTS_FILE_PATH: './defaults.json'
+ });
+ TemplateFile.render(path.join(ROOT, 'framework', 'build.gradle'), path.join(nestedCordovaLibPath, 'build.gradle'), {
Review comment:
The only file that seems to actually make use of interpolating template variables is `project.properties`. Did you refactor something and this is leftovers or did I not get something?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org