netdev - RE: [PATCH net] tools: ynl: add the Python requirements.txt file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BN6PR11MB41772BEF5321C0ECEE4B0A2BE3809@BN6PR11MB4177.namprd11.prod.outlook.com>
Date:   Mon, 20 Mar 2023 19:03:01 +0000
From:   "Michalik, Michal" <michal.michalik@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
Subject: RE: [PATCH net] tools: ynl: add the Python requirements.txt file

From: Jakub Kicinski <kuba@...nel.org> 
Sent: Thursday, March 16, 2023 5:40 AM
> 
> On Tue, 14 Mar 2023 17:07:58 +0100 Michal Michalik wrote:
>> It is a good practice to state explicitely which are the required Python
>> packages needed in a particular project to run it. The most commonly
>> used way is to store them in the `requirements.txt` file*.
>> 
>> *URL: https://pip.pypa.io/en/stable/reference/requirements-file-format/
>> 
>> Currently user needs to figure out himself that Python needs `PyYAML`
>> and `jsonschema` (and theirs requirements) packages to use the tool.
>> Add the `requirements.txt` for user convenience.
>> 
>> How to use it:
>> 1) (optional) Create and activate empty virtual environment:
>>   python3.X -m venv venv3X
>>   source ./venv3X/bin/activate
>> 2) Install all the required packages:
>>   pip install -r requirements.txt
>>     or
>>   python -m pip install -r requirements.txt
>> 3) Run the script!
>> 
>> The `requirements.txt` file was tested for:
>> * Python 3.6
>> * Python 3.8
>> * Python 3.10
> 
> Is this very useful? IDK much about python, I'm trying to use only
> packages which are commonly installed on Linux systems. jsonschema
> is an exception, so I've added the --no-schema option to cli.py to
> avoid it.
> 

Well, that is a very subjective question - I will present you my point of
view and leave a decision if that is useful or not to you. I'm totally
okay with rejecting this patch if you decide so.

First of all, I love the idea behind this script - thanks for creating it.
As a experienced Python user I looked at the scripts and see no file with
requirements so I assumed the default libs would be enough. First run
proved me wrong, exception on `jsonschema` missing*. No big deal, I
installed it. Second run, exception on `import yaml`. Still, no big deal
because I know that most probably you meant `PyYAML`. I don't really like
the idea that user need to either have this knowledge or look for it, so
I proposed this good practice file containing the required libs.

If you asked me, I feel it's useful - but again, I'm really okay to drop
this patch.

*yes, you added `no-schema` and `schema` with no defaults, it would be
enough to leave only `schema` defaulting to `None` or empty string and
users would not ran into the problem I faced, because right now default
behavior is to need this package

>> diff --git a/tools/net/ynl/requirements.txt b/tools/net/ynl/requirements.txt
>> new file mode 100644
>> index 0000000..2ad25d9
>> --- /dev/null
>> +++ b/tools/net/ynl/requirements.txt
>> @@ -0,0 +1,7 @@
>> +attrs==22.2.0
>> +importlib-metadata==4.8.3
>> +jsonschema==4.0.0
>> +pyrsistent==0.18.0
>> +PyYAML==6.0
>> +typing-extensions==4.1.1
>> +zipp==3.6.0
> 
> Why the == signs? Do we care about the version of any of these?
> Also, there's a lot more stuff here than I thought I'm using.
> What's zipp and why typing? Did I type something and forgot? :S
> 

I cannot (you probably also not) guarantee the consistency of the API of
particular libraries. For example, maybe somebody would change the calls
in `jsonschema 5.0.0` in the future. In this case - your script will
still work (will use 4.0.0). I suggested this packages versions, because
I checked them for Python 3.6, 3.8, 3.10. I'm fine with removing the
exact versions, but please don't blame me in the future if any of the
maintainters of those libs changes their API.

No, you did not forget about anything (besides the PyYAML that you didn't
mention above). There is more than you expect because `PyYAML` and
`jsonschema` have their own dependencies. If you like experimenting,
please create clean environment then do `pip list` and see the list,
then install only `PyYAML` and `jsonschema` and print the list again.