[ 
https://issues.apache.org/jira/browse/CB-10654?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15155406#comment-15155406
 ] 

ASF GitHub Bot commented on CB-10654:
-------------------------------------

Github user bso-intel commented on a diff in the pull request:

    https://github.com/apache/cordova-lib/pull/395#discussion_r53545199
  
    --- Diff: cordova-lib/src/cordova/platform.js ---
    @@ -79,148 +79,156 @@ function addHelper(cmd, hooksRunner, projectRoot, 
targets, opts) {
         var platformsDir = path.join(projectRoot, 'platforms');
         shell.mkdir('-p', platformsDir);
     
    -    return hooksRunner.fire('before_platform_' + cmd, opts)
    -    .then(function() {
    -        return promiseutil.Q_chainmap(targets, function(target) {
    -            // For each platform, download it and call its helper script.
    -            var parts = target.split('@');
    -            var platform = parts[0];
    -            var spec = parts[1];
    -
    -            return Q.when().then(function() {
    -                if (!(platform in platforms)) {
    -                    spec = platform;
    -                    platform = null;
    -                }
    +    return promiseutil.Q_chainmap(targets, function(target) {
    +        // For each platform, download it and call its helper script.
    +        var parts = target.split('@');
    +        var platform = parts[0];
    +        var spec = parts[1];
    +        var platDetails = '';
    +
    +        return Q.when().then(function() {
    +            if (!(platform in platforms)) {
    +                spec = platform;
    +                platform = null;
    +            }
     
    -                if(platform === 'amazon-fireos') {
    -                    events.emit('warn', 'amazon-fireos has been 
deprecated. Please use android instead.');
    -                }
    -                if(platform === 'wp8') {
    -                    events.emit('warn', 'wp8 has been deprecated. Please 
use windows instead.');
    -                }
    -                if (platform && !spec && cmd == 'add') {
    -                    events.emit('verbose', 'No version supplied. 
Retrieving version from config.xml...');
    -                    spec = getVersionFromConfigFile(platform, cfg);
    -                }
    +            if(platform === 'amazon-fireos') {
    +                events.emit('warn', 'amazon-fireos has been deprecated. 
Please use android instead.');
    +            }
    +            if(platform === 'wp8') {
    +                events.emit('warn', 'wp8 has been deprecated. Please use 
windows instead.');
    +            }
    +            if (platform && !spec && cmd == 'add') {
    +                events.emit('verbose', 'No version supplied. Retrieving 
version from config.xml...');
    +                spec = getVersionFromConfigFile(platform, cfg);
    +            }
    +
    +            // If --save/autosave on && no version specified, use the 
pinned version
    +            // e.g: 'cordova platform add android --save', 'cordova 
platform update android --save'
    +            if( (opts.save || autosave) && !spec ){
    +                spec = platforms[platform].version;
    +            }
     
    -                // If --save/autosave on && no version specified, use the 
pinned version
    -                // e.g: 'cordova platform add android --save', 'cordova 
platform update android --save'
    -                if( (opts.save || autosave) && !spec ){
    -                    spec = platforms[platform].version;
    +            if (spec) {
    +                var maybeDir = cordova_util.fixRelativePath(spec);
    +                if (cordova_util.isDirectory(maybeDir)) {
    +                    return getPlatformDetailsFromDir(maybeDir, platform);
    +                }
    +            }
    +            return downloadPlatform(projectRoot, platform, spec, opts);
    +        }).then(function(platformDetails) {
    +            platDetails = platformDetails;
    +            var hookOpts = {
    +                platforms :[platDetails.platform],
    +                nohooks :[opts.nohooks]
    +            };
    +            return hooksRunner.fire('before_platform_' + cmd, hookOpts);
    +        }).then(function() {
    +            platform = platDetails.platform;
    +            var platformPath = path.join(projectRoot, 'platforms', 
platform);
    +            var platformAlreadyAdded = fs.existsSync(platformPath);
    +
    +            if (cmd == 'add') {
    +                if (platformAlreadyAdded) {
    +                    throw new CordovaError('Platform ' + platform + ' 
already added.');
                     }
     
    -                if (spec) {
    -                    var maybeDir = cordova_util.fixRelativePath(spec);
    -                    if (cordova_util.isDirectory(maybeDir)) {
    -                        return getPlatformDetailsFromDir(maybeDir, 
platform);
    -                    }
    +                // Remove the <platform>.json file from the plugins 
directory, so we start clean (otherwise we
    +                // can get into trouble not installing plugins if someone 
deletes the platform folder but
    +                // <platform>.json still exists).
    +                removePlatformPluginsJson(projectRoot, target);
    +            } else if (cmd == 'update') {
    +                if (!platformAlreadyAdded) {
    +                    throw new CordovaError('Platform "' + platform + '" is 
not yet added. See `' +
    +                        cordova_util.binname + ' platform list`.');
                     }
    -                return downloadPlatform(projectRoot, platform, spec, opts);
    -            }).then(function(platDetails) {
    -                platform = platDetails.platform;
    -                var platformPath = path.join(projectRoot, 'platforms', 
platform);
    -                var platformAlreadyAdded = fs.existsSync(platformPath);
    +            }
     
    -                if (cmd == 'add') {
    -                    if (platformAlreadyAdded) {
    -                        throw new CordovaError('Platform ' + platform + ' 
already added.');
    -                    }
    +            var options = {
    +                // We need to pass a platformDetails into update/create
    +                // since PlatformApiPoly needs to know something about
    +                // platform, it is going to create.
    +                platformDetails: platDetails,
    +                link: opts.link
    +            };
     
    -                    // Remove the <platform>.json file from the plugins 
directory, so we start clean (otherwise we
    -                    // can get into trouble not installing plugins if 
someone deletes the platform folder but
    -                    // <platform>.json still exists).
    -                    removePlatformPluginsJson(projectRoot, target);
    -                } else if (cmd == 'update') {
    -                    if (!platformAlreadyAdded) {
    -                        throw new CordovaError('Platform "' + platform + 
'" is not yet added. See `' +
    -                            cordova_util.binname + ' platform list`.');
    -                    }
    -                }
    +            if (config_json && config_json.lib && 
config_json.lib[platform] &&
    +                config_json.lib[platform].template) {
    +                options.customTemplate = 
config_json.lib[platform].template;
    +            }
     
    -                var options = {
    -                    // We need to pass a platformDetails into update/create
    -                    // since PlatformApiPoly needs to know something about
    -                    // platform, it is going to create.
    -                    platformDetails: platDetails,
    -                    link: opts.link
    -                };
    +            events.emit('log', (cmd === 'add' ? 'Adding ' : 'Updating ') + 
platform + ' project...');
     
    -                if (config_json && config_json.lib && 
config_json.lib[platform] &&
    -                    config_json.lib[platform].template) {
    -                    options.customTemplate = 
config_json.lib[platform].template;
    +            var PlatformApi;
    +            try {
    +                // Try to get PlatformApi class from platform
    +                // Get an entry point for platform package
    +                var apiEntryPoint = require.resolve(platDetails.libDir);
    +                // Validate entry point filename. This is required since 
most of platforms
    +                // defines 'main' entry in package.json pointing to 
bin/create which is
    +                // basically a valid NodeJS script but intended to be used 
as a regular
    +                // executable script.
    +                if (path.basename(apiEntryPoint) === 'Api.js') {
    +                    PlatformApi = require(apiEntryPoint);
    +                    events.emit('verbose', 'PlatformApi successfully found 
for platform ' + platform);
                     }
    -
    -                events.emit('log', (cmd === 'add' ? 'Adding ' : 'Updating 
') + platform + ' project...');
    -
    -                var PlatformApi;
    -                try {
    -                    // Try to get PlatformApi class from platform
    -                    // Get an entry point for platform package
    -                    var apiEntryPoint = 
require.resolve(platDetails.libDir);
    -                    // Validate entry point filename. This is required 
since most of platforms
    -                    // defines 'main' entry in package.json pointing to 
bin/create which is
    -                    // basically a valid NodeJS script but intended to be 
used as a regular
    -                    // executable script.
    -                    if (path.basename(apiEntryPoint) === 'Api.js') {
    -                        PlatformApi = require(apiEntryPoint);
    -                        events.emit('verbose', 'PlatformApi successfully 
found for platform ' + platform);
    -                    }
    -                } catch (e) {
    -                } finally {
    -                    if (!PlatformApi) {
    -                        events.emit('verbose', 'Failed to require 
PlatformApi instance for platform "' + platform +
    -                            '". Using polyfill instead.');
    -                        PlatformApi = 
require('../platforms/PlatformApiPoly');
    -                    }
    +            } catch (e) {
    +            } finally {
    +                if (!PlatformApi) {
    +                    events.emit('verbose', 'Failed to require PlatformApi 
instance for platform "' + platform +
    +                        '". Using polyfill instead.');
    +                    PlatformApi = require('../platforms/PlatformApiPoly');
                     }
    +            }
     
    -                var destination = path.resolve(projectRoot, 'platforms', 
platform);
    -                var promise = cmd === 'add' ?
    -                    PlatformApi.createPlatform.bind(null, destination, 
cfg, options, events) :
    -                    PlatformApi.updatePlatform.bind(null, destination, 
options, events);
    +            var destination = path.resolve(projectRoot, 'platforms', 
platform);
    +            var promise = cmd === 'add' ?
    +                PlatformApi.createPlatform.bind(null, destination, cfg, 
options, events) :
    +                PlatformApi.updatePlatform.bind(null, destination, 
options, events);
     
    -                return promise()
    -                .then(function() {
    -                    if (cmd == 'add') {
    -                        return installPluginsForNewPlatform(platform, 
projectRoot, opts);
    -                    }
    -                })
    -                .then(function () {
    -                    // Call prepare for the current platform.
    -                    var prepOpts = {
    -                        platforms :[platform],
    -                        searchpath :opts.searchpath
    -                    };
    -                    return require('./cordova').raw.prepare(prepOpts);
    +            return promise()
    +            .then(function() {
    +                if (cmd == 'add') {
    +                    return installPluginsForNewPlatform(platform, 
projectRoot, opts);
    +                }
    +            })
    +            .then(function () {
    +                // Call prepare for the current platform.
    +                var prepOpts = {
    +                    platforms :[platform],
    +                    searchpath :opts.searchpath
    +                };
    +                return require('./cordova').raw.prepare(prepOpts);
                     })
                     .then(function() {
    -                    var saveVersion = !spec || semver.validRange(spec, 
true);
    -
    -                    // Save platform@spec into platforms.json, where 
'spec' is a version or a soure location. If a
    -                    // source location was specified, we always save that. 
Otherwise we save the version that was
    -                    // actually installed.
    -                    var versionToSave = saveVersion ? platDetails.version 
: spec;
    -                    events.emit('verbose', 'saving ' + platform + '@' + 
versionToSave + ' into platforms.json');
    -                    platformMetadata.save(projectRoot, platform, 
versionToSave);
    -
    -                    if(opts.save || autosave){
    -                        // Similarly here, we save the source location if 
that was specified, otherwise the version that
    -                        // was installed. However, we save it with the "~" 
attribute (this allows for patch updates).
    -                        spec = saveVersion ? '~' + platDetails.version : 
spec;
    -
    -                        // Save target into config.xml, overriding already 
existing settings
    -                        events.emit('log', '--save flag or autosave 
detected');
    -                        events.emit('log', 'Saving ' + platform + '@' + 
spec + ' into config.xml file ...');
    -                        cfg.removeEngine(platform);
    -                        cfg.addEngine(platform, spec);
    -                        cfg.write();
    -                    }
    -                });
    +                var saveVersion = !spec || semver.validRange(spec, true);
    +
    +                // Save platform@spec into platforms.json, where 'spec' is 
a version or a soure location. If a
    +                // source location was specified, we always save that. 
Otherwise we save the version that was
    +                // actually installed.
    +                var versionToSave = saveVersion ? platDetails.version : 
spec;
    +                events.emit('verbose', 'saving ' + platform + '@' + 
versionToSave + ' into platforms.json');
    +                platformMetadata.save(projectRoot, platform, 
versionToSave);
    +
    +                if(opts.save || autosave){
    +                    // Similarly here, we save the source location if that 
was specified, otherwise the version that
    +                    // was installed. However, we save it with the "~" 
attribute (this allows for patch updates).
    +                    spec = saveVersion ? '~' + platDetails.version : spec;
    +
    +                    // Save target into config.xml, overriding already 
existing settings
    +                    events.emit('log', '--save flag or autosave detected');
    +                    events.emit('log', 'Saving ' + platform + '@' + spec + 
' into config.xml file ...');
    +                    cfg.removeEngine(platform);
    +                    cfg.addEngine(platform, spec);
    +                    cfg.write();
    +                }
    +                var hookOpts = {
    +                    platforms :[platDetails.platform],
    +                    nohooks :[opts.nohooks]
    --- End diff --
    
    The same thing. Please remove bracket [ ] here.


> _platform_add hooks not executed when platform added from repo or directory
> ---------------------------------------------------------------------------
>
>                 Key: CB-10654
>                 URL: https://issues.apache.org/jira/browse/CB-10654
>             Project: Apache Cordova
>          Issue Type: Bug
>          Components: CordovaLib
>    Affects Versions: Master
>            Reporter: Tony Homer
>            Assignee: Tony Homer
>         Attachments: CB-10654.zip
>
>
> platform-specific before_platform_add and after_platform_add hooks are not 
> executed when platforms are added from directory or repo.  
> e.g., hooks defined in config.xml as follows
> {code}
> <platform name="android">
> <hook type="before_platform_add" 
> src="scripts/before_platform_add/android/001_test.js" />
> <hook type="after_platform_add" 
> src="scripts/after_platform_add/android/001_test.js" />
> </platform>
> {code}
> This is because the matching logic in platform.js depends on the targets 
> specified in command line arguments.
> It seems that the matching logic should be deferred until the platform can be 
> determined via getPlatformDetailsFromDir.
> This would require that the before_platform_add hook not be executed until 
> after the platform has been downloaded.
> Steps to reproduce
> # create a project with platform-specific hooks for before_platform_add 
> and/or after_platform_add defined in config.xml
> # add platform from npm, e.g.
> {code}
> cordova platform add android
> {code}
> # hooks get executed
> # add platform from repo or local dir, e.g.
> {code}
> cordova platform add https://github.com/apache/cordova-android
> cordova platform add my-local-copy-of-cordova-android
> {code}
> # hooks do not get executed



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

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

Reply via email to