Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[bug] order of command line params now matters #6719

Closed
keysight-daryl opened this issue Mar 23, 2020 · 21 comments
Closed

[bug] order of command line params now matters #6719

keysight-daryl opened this issue Mar 23, 2020 · 21 comments
Assignees
@memsharded
Labels
stage: triaging type: question
Projects
CLI

Comments

@keysight-daryl
Copy link

Screen Shot 2020-03-23 at 1 57 28 PM

Environment Details (include every applicable attribute)

  • Operating System+version: Win10 latest
  • Compiler+version: visual studio 2017 latest
  • Conan version: 1.23.0
  • Python version: 3.8.2

Steps to reproduce (Include if Applicable)

put the repo id (keysght/stable) after some other options in the command line

Logs (Executed commands with output) (Include/Attach if Applicable)

attached
@keysight-daryl
Copy link
Author

moving keysight/stable to just after the '.' fixes it however i have dozens of scripts that would need fixed to resolve this and i don't think it should matter the order if it didn't in the past.

@memsharded memsharded self-assigned this Mar 23, 2020
@memsharded
Copy link
Member

Hi @keysight-daryl

Thanks for reporting this. Up to my knowledge nothing has changed in the command line to cause this issue. From what version are you upgrading? Could you please check that the upgrade is only Conan and not anything in the environment that could affect, as dependencies (pip list them, please) or the python version itself?

In any case, I have found that the python argparse module is not rock solid regarding positional parameters, this is why I always recommend always to define the positional arguments first in the command line, and then the optional ones (even if that is not the convention in linux).

Please let me know the above, let's try to identify if there has been some regression that we could fix on our side.

@keysight-daryl
Copy link
Author

Before the upgrade I was using python 3.7.x and i don't have records of exact conan version but it was probably around 1.18 or so. yeah this is being run under windows but for the entire previous history of using conan as such it never complained about the order of these args and unfortunately i have actually more then dozens (closer to a hundred) scripts specified the way it is.

@keysight-daryl
Copy link
Author

keysight-daryl commented Mar 23, 2020 •
edited

upgraded (broken) environment is cmake 3.17.0 + python 3.8.2 + conan 1.23.0 running under latest windows 10

@memsharded
Copy link
Member

There is a relatively easy test. In the same environment that you are running with python 3.8, do a forced pip install conan==1.18.0 and try the same command, that would help if you could do that.

@Johnnyxy
Copy link

Johnnyxy commented Mar 24, 2020 •
edited

Could this be related to an "issue" I pointed out too? See here: conan-io/docs#1051 (comment)

Despite the content of the docs one should not put the path/reference behind any command line options.

@memsharded
Copy link
Member

Yes, exactly, in any case, the positional args should be the very first, or the very last, but it is quite discourage to put them in between optional arguments, up to my knowledge.

Please @keysight-daryl let me know if you managed to test in your env with 1.18.0.

@keysight-daryl
Copy link
Author

keysight-daryl commented Mar 24, 2020 via email

@memsharded
Copy link
Member

yes this worked fine up until my upgrade to the latest so 1.18 with python 3.7 should not cause this error.

Yes, I got that, but not sure if I was clearly pointing the possible error. What I am saying is that the argparse standard library in Python 3.8 might be behaving differently, and it seems that there is probably nothing we can do in the Conan codebase, besides saying in the docs that the positional arguments should be either in the beginning or at the end of the command line, but not in the middle. This is what it would be useful to check if Conan 1.18 works fine with Python 3.8

@keysight-daryl
Copy link
Author

keysight-daryl commented Mar 24, 2020 via email

@memsharded
Copy link
Member

Hi @keysight-daryl

I have done the check:

(conanpip38) λ python --version                                                                                                                                         
Python 3.8.1                                                                                                                                                            

(conanpip38) λ conan --version
Conan version 1.18.5
                                                                                                                                                                                                                                                                                                                                                                                                                                         
(conanpip38) λ conan create . -tf=test_package -pr=default user/testing --build=missing                                                                                                                                                                                           
usage: conan create [-h] [-j JSON] [-k] [-kb] [-ne] [-tbf TEST_BUILD_FOLDER] [-tf TEST_FOLDER] [-m [MANIFESTS]] [-mi [MANIFESTS_INTERACTIVE]] [-v [VERIFY]] [-b [BUILD]]
                    [-e ENV] [-o OPTIONS] [-pr PROFILE] [-r REMOTE] [-s SETTINGS] [-u] [-l [LOCKFILE]]                                                                  
                    path [reference]                                                                                                                                    
conan create: error: unrecognized arguments: user/testing                                                                                                               
ERROR: Exiting with code: 2

So it seems confirmed this is not a bug in Conan, and Conan has not changed this behavior since Conan 1.18. This is a change in the python ecosystem and how python interprets arguments with its argparse module. The only solutions seems trying to stick to the calling conventions, either the automatic --help one that puts all the positional arguments last, or the convention that is used in the conan docs, and put all the positional arguments first. But not interleaving the positional arguments between the optional arguments.

@Johnnyxy
Copy link

Johnnyxy commented Mar 25, 2020 •
edited

@memsharded
As with every well defined software one should always resort to the official documentation (in what form or shape it ever is). As it mostly states the intentions of the developers. Bugs and other mistakes can interfere with that but normally all fixes are going to correct that.

As you said the docs are parsed and generated and you cannot change the parameter order, it would be helpful to make some note at the commands like conan remove etc. to strongly recommend the correct usage. Otherwise users are playing around and use what is "working" but this does not necessarily mean that it is the correct way ( own experience :) ).

@memsharded
Copy link
Member

Hi @Johnnyxy

Yes, agree. If @keysight-daryl agrees too, I'll move this to the docs to try to clarify this issue there,

@keysight-daryl
Copy link
Author

keysight-daryl commented Mar 26, 2020 via email

@memsharded
Copy link
Member

I am sorry, I am afraid --repo=keysight/stable doesn't exist. It is not related to a repo at all, it is the user/channel, but it can be uploaded/downloaded from any repo. And in any case, it would require to modify all your command line anyway, so it is not that it would avoid changing your command line invocation code.

@keysight-daryl
Copy link
Author

keysight-daryl commented Mar 26, 2020 via email

@Johnnyxy
Copy link

It depends. --whatever are parameters like -o or -s which modify the behaviour of or give hints to a command. But the reference or user/channel on several commands is a necessity to function at all. If the user/channel would be specified with --whatever user/channel one could get the impression that it is optional to specify it.

@keysight-daryl
Copy link
Author

keysight-daryl commented Mar 26, 2020 via email

@memsharded
Copy link
Member

could be either right? it is required but specified as either without the --flag and order is important or with and order is not important.

The conan command line has some flaws that need to be improved, and we will when we have the occasion to break (Conan 2.0). But I am sorry, but that would be bad command line or api design. You cannot have 2 ways to introduce the same information both as positional and optional in the same command. What if they provide the two, but they are different? Having 2 ways to do the same thing should be avoided, it should be one or the other. Positional if it is mandatory, and optional if it is optional.

@keysight-daryl
Copy link
Author

keysight-daryl commented Mar 27, 2020 via email

@memsharded
Copy link
Member

This proved to be a bug in Python argparse itself python/cpython#53584, also happened in conan-io/docs#3629

Closing this as out of scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@memsharded memsharded

Labels
stage: triaging type: question
Projects
CLI
Awaiting triage
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Johnnyxy @memsharded @keysight-daryl