caioaamaral commented 3 years ago

Signed-off-by: Caio Amaral caioaamaral@gmail.com

🦟 Bug fix

Fixes #36

Summary

Relax the --force-version verification method. Now it's not necessary to specify the fully-qualified version, instead if only the major version is passed, commands will be tested against the major version only.

Checklist

[ ] Signed all commits for DCO
[ ] Added tests
[ ] Updated documentation (as needed)
[ ] Updated migration guide (as needed)
[ ] codecheck passed (See contributing)
[ ] All tests passed (See test coverage)
[ ] While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

caioaamaral commented 3 years ago

I confess I've got a bit lost on how to write a test for this fix (neither figure out where are the tests), but if desired I can write one.

caioaamaral commented 3 years ago

@caguero friendly ping

caioaamaral commented 3 years ago

@chapulina @caguero friendly ping

caguero commented 3 years ago

@caioaamaral , I'll review your PR this week. Thanks for your contribution!

caioaamaral commented 3 years ago

Could you also check the discussion on issue #36 please? There's a good suggestion by @chapulina for keeping just the --force-version argument but allowing multiple values:
* Just a major version. E.g.: `--force-version 9`

* Major and minor. E.g.: `--force-version 9.1`

* Full version (current behavior). E.g.: `--force-version 9.1.0`

This change is expected to behave just like this. I'm splitting the version used as input into different fields (based on the number of '.'), then just those fields will be used when checking against the library version.

Ex:

required_version = '9.1'
library version = '9.1.3'

will check only two first fields of version, if they matches then this is the one that will be used.

This is achieved by this change on the current implementation:

    required_version = required_version.split('.')
    version = version.split('.')
    next unless required_version.zip(version).map {|required, current| required == current}.all?

if required_version has different size than version, required_version.zip(version) will have required_version size, ignoring the last fields from version

caguero commented 3 years ago

Could you also check the discussion on issue #36 please? There's a good suggestion by @chapulina for keeping just the --force-version argument but allowing multiple values:
* Just a major version. E.g.: `--force-version 9`

* Major and minor. E.g.: `--force-version 9.1`

* Full version (current behavior). E.g.: `--force-version 9.1.0`
This change is expected to behave just like this. I'm splitting the version used as input into different fields (based on the number of '.'), then just those fields will be used when checking against the library version.

Ex:
required_version = '9.1'
library version = '9.1.3'
will check only two first fields of version, if they matches then this is the one that will be used.

This is achieved by this change on the current implementation:
    required_version = required_version.split('.')
    version = version.split('.')
    next unless required_version.zip(version).map {|required, current| required == current}.all?
if required_version has different size than version, required_version.zip(version) will have required_version size, ignoring the last fields from version

Thanks for the clarification!

caioaamaral commented 3 years ago

Do you get the same error?

My mistake, sorry for the noise. Just forced push edd8b16 which will solve this problem.

Running locally using gazebo command (please ignore the XDG warning, gazebo opened just fine): Edit: gazebo version is 3.8.0

orise@caioaamaral-Nabuchadnezzar:bin$ ./ign gazebo --force-version 3
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-orise'
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign gazebo --force-version 3.1
Version error: I cannot find this command in version [3.1].
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign gazebo --force-version 3.8
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-orise'
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign gazebo --force-version 3.8.0
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-orise'
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign gazebo --force-version 3.8.1
Version error: I cannot find this command in version [3.8.1].

caioaamaral commented 3 years ago

Thanks for the changes! It works for me as expected when I provide a version that matches my installed library. However it produces an exception when the library is not found, as opposed to show the error message "I cannot find the library...". E.g.:

Strange, I can't reproduce this error:

orise@caioaamaral-Nabuchadnezzar:bin$ ./ign topic -l --versions
8.2.0
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign topic -l --force-version 12
Version error: I cannot find this command in version [12].
orise@caioaamaral-Nabuchadnezzar:bin$ ./ign topic -l --force-version 12.1.2
Version error: I cannot find this command in version [12.1.2].

Maybe your binary is missing my last update?

caguero commented 3 years ago

Strange, I can't reproduce this error:

I think I know what's happening. My guess is that you only have one version of Ignition Transport installed. The problem seems to be here:

commands[ARGV[0]].each_with_index do |version, index|
    required_version = required_version.split('.')

Originally, required_version is a String and you can call split(). However you're overwriting required_version with the result of the split(), converting it to an Array. The next time you iterate in the loop (only if you have more than one version), split fails because it's not declared for Array types.

My suggestion is to use a separate local variable for the result of split(). As a bonus point, you'll also avoid to call join() later in case you need to print the error message.

caioaamaral commented 3 years ago

Strange, I can't reproduce this error:

I think I know what's happening. My guess is that you only have one version of Ignition Transport installed. The problem seems to be here:
commands[ARGV[0]].each_with_index do |version, index|
    required_version = required_version.split('.')
Originally, required_version is a String and you can call split(). However you're overwriting required_version with the result of the split(), converting it to an Array. The next time you iterate in the loop (only if you have more than one version), split fails because it's not declared for Array types.

My suggestion is to use a separate local variable for the result of split(). As a bonus point, you'll also avoid to call join() later in case you need to print the error message.

Got it, thanks for the clarification. Edit: e473b66 applies your suggestion

caguero commented 3 years ago

I confess I've got a bit lost on how to write a test for this fix (neither figure out where are the tests), but if desired I can write one.

It's OK, for now we only have a static code checker test. There was a minor issue fixed in ce44884.

gazebosim / gz-tools

Relax version verification #44

🦟 Bug fix

Summary

Checklist