You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by Mitko-Kerezov <gi...@git.apache.org> on 2016/01/06 17:11:07 UTC
[GitHub] cordova-android pull request: Parse options correctly
GitHub user Mitko-Kerezov opened a pull request:
https://github.com/apache/cordova-android/pull/248
Parse options correctly
Fix 1
Options have already passed through `nopt` module in the build file. Nopt successfully parses all options which results in `argv.remain` being an empty array.
This ultimately leads to a bug in the `parseOpts` function where the options undergo a second parsing from nopt, whereas they are actually ready to be consumed.
The end result is - this options are not respected at all.
Fix 2
parseOpts function referenced `this.root` but the `this` object is not the `new Api` as expected. This leads to an exception upon calling `path.relative(this.root, ...)`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Icenium/cordova-android kerezov/fix-parse-options
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-android/pull/248.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #248
----
commit c1651fd6dd49c6a5a46eaf8a88400b06fa164ab1
Author: Dimitar Kerezov <di...@telerik.com>
Date: 2016-01-06T15:26:06Z
Parse options correctly
Fix #1
Options have already passed through `nopt` module in the build file. Nopt successfully parses all options which results in `argv.remain` being an empty array.
This ultimately leads to a bug in the `parseOpts` function where the options undergo a second parsing from nopt, whereas they are actually ready to be consumed.
Fix #2
parseOpts function referenced `this.root` but the `this` object is not the `new Api` as expected. This leads to an exception upon calling `path.relative(this.root, ...)`.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: Parse options correctly
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-android/pull/248#issuecomment-175576742
@Mitko-Kerezov, my bad, i missed you name in CLA list
Regarding the
> the usage of -- basically suggests "I want to stop passing options and start passing actual arguments - filenames, etc"
I'd say it tells "do not consider the options behind me, just pass them to underlying script" (see, for example, [istanbul docs](https://github.com/gotwarlost/istanbul#the-cover-command) and [`npm run` help](https://docs.npmjs.com/cli/run-script#description)).
There is two possible ways to run build: `platforms/android/cordova/build --ant` and `cordova build android -- --ant` usage. In the first case `--` delimiter is not needed, indeed, because you're running the script directly, but in the second it means that CLI will ignore these options and just pass them to android's `build`.
It is also still necessary for compatibility reasons (IMO, changing the way how CLI arguments are treated is a breaking change).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: Parse options correctly
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-android/pull/248#issuecomment-173500760
The second fix LGTM.
Regarding the first - looks like it breaks the scenario when platform-specific options are passed behind `--` arguments delimiter. I.e. with this patch applied the following command `cordova build android -- --ant` still uses gradle.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: Parse options correctly
Posted by Mitko-Kerezov <gi...@git.apache.org>.
Github user Mitko-Kerezov commented on the pull request:
https://github.com/apache/cordova-android/pull/248#issuecomment-176044525
@vladimir-kotikov
I've restructured the fixes and have separated them in two commits.
Regarding the options I've considered your words and have taken a different approach.
I didn't know that `cordova build android -- --ant` was meant to be used that way, however `platforms/android/cordova/build --ant` does not work for `cordova-android@5.0.0`.
It does work however for the previous `cordova-android@4.1.1`.
I've backtracked a bit in the commits before your [major refactoring](400282282f21c07bda5527e4c2266810fe01da3f) and have come across the [previous implementation](https://github.com/apache/cordova-android/blob/789c505a88a6bd2c7c5be596908c85681e457132/bin/templates/cordova/build#L36).
By passing `args.slice(2)` all options have been passed to the build script. This is why I've passed `original` instead of `remain`. Moreover nopt seems to parse all options and leaves nothing inside remain even if `--` delimiter is passed which is somewhat strange but something to live with I guess.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: Parse options correctly
Posted by Mitko-Kerezov <gi...@git.apache.org>.
Github user Mitko-Kerezov commented on the pull request:
https://github.com/apache/cordova-android/pull/248#issuecomment-169372671
Ping @vladimir-kotikov
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: Parse options correctly
Posted by Mitko-Kerezov <gi...@git.apache.org>.
Github user Mitko-Kerezov commented on the pull request:
https://github.com/apache/cordova-android/pull/248#issuecomment-169964965
Ping @infil00p for review
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: Parse options correctly
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-android/pull/248#issuecomment-176136907
Wohoo! Good to see this is getting committed!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: Parse options correctly
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-android/pull/248#issuecomment-175224220
This looks like a great fix to merge. @Mitko-Kerezov Did you get a chance to file the ICLA?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: Parse options correctly
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-android/pull/248
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: Parse options correctly
Posted by Mitko-Kerezov <gi...@git.apache.org>.
Github user Mitko-Kerezov commented on the pull request:
https://github.com/apache/cordova-android/pull/248#issuecomment-175496415
@nikhilkh I've already filed the ICLA some time in 2015 and have already contributed to cordova. (Can't find the email though but I have merged pull requests)
@vladimir-kotikov regarding your comment, I don't believe `cordova build android -- --ant` should work at all. The `--` delimiter stops the options parsing for `nopt` module and places the remaining args in `argv.remain`. This worked prior to the fix, however this is faulty behavior IMO.
Basically we'd either want `cordova build android --ant` or `cordova build android -- --ant` to work. The former is the right way IMO, because in the latter the usage of `--` basically suggests "I want to stop passing options and start passing actual arguments - filenames, etc"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: Parse options correctly
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-android/pull/248#issuecomment-176068570
Verified and it seems to work fine now with all possible combinations of `--` and other args (i tested with `--ant`, `--keystore` and `--alias` )
Thanks, @Mitko-Kerezov
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org