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.

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

Make colorama dependency optional #1180

Merged
merged 2 commits into from
Oct 21, 2020

Conversation

joshuagl
Copy link
Member

Fixes N/A

Description of the changes being introduced by the pull request:

  • Switch repo script from using colorama directly to using the TERM_ values defined in securesystemslib.interface. These values are set automatically depending on whether colorama is importable, and therefore make the colorama dependency an optional/soft dependency
  • Remove colorama from the requirements

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

Instead of using colorama directly for terminal colours, use the
constants in securesystemslib.interface which map to colorama colours
IFF colorama is installed.

This change results in a red password prompt when colorama is installed
and a standard terminal output coloured prompt when colorama is not
installed.

Signed-off-by: Joshua Lock <jlock@vmware.com>
The repo script was the only user and can now do the right thing when
colorama isn't available in the environment.

Signed-off-by: Joshua Lock <jlock@vmware.com>
@joshuagl joshuagl merged commit b570723 into theupdateframework:develop Oct 21, 2020
@joshuagl joshuagl deleted the joshuagl/nocolour branch October 21, 2020 10:06
@lukpueh
Copy link
Member

lukpueh commented Oct 22, 2020

Sorry for the late comment. But I don't think we actually want to remove coloroma from the requirements files. As (maybe not clearly enough) stated in requirements.txts doc header, the idea is to use requirements.txt to generate a requirements-pinned.txt with all required and optional, immediate and transitive dependencies, to be auto-bumped and auto-tested via Dependabot.

Now, if we remove colorama from these files Dependabot won't notify us about breaking updates.

We use setup.py to encode the minimal set of requirements, which already didn't list coloroma prior to this PR. Also note that the crypto and pynacl extra markers, which indicate optional dependencies, are not in setup.py (but still in the requirements files).

What do you think... @joshuagl, @jku? I'm happy to once more revisit the requirements handling (see #978 and #982 for prior work).

@jku
Copy link
Member

jku commented Oct 22, 2020

so I think you're making a lot of sense (I had missed the idea of generating requirements-pinned.txt completely).

The only thing I'm missing is why do we want colorama as a dependency at all -- as far as I can tell where now using securesystemslib features only, there's essentially no colorama anywhere?

EDIT: or is the idea to document a version for every possible (even transient optional) dependency?

@lukpueh
Copy link
Member

lukpueh commented Oct 22, 2020

so I think you're making a lot of sense (I had missed the idea of generating requirements-pinned.txt completely).

I admit that the documentation is not ideal.

@lukpueh
Copy link
Member

lukpueh commented Oct 22, 2020

EDIT: or is the idea to document a version for every possible (even transient optional) dependency?

I think pip-tools goes one level deep into transitive dependencies. IMO we should at least test securesystemslib, which is basically a part of TUF in a separate package, with all its optional dependencies, because they were added for TUF.

@lukpueh
Copy link
Member

lukpueh commented Oct 22, 2020

If let's say six has an optional dependency that we are not aware of and don't care for, we don't need to test it.

@lukpueh
Copy link
Member

lukpueh commented Oct 22, 2020

That said, I think the easiest thing to do would be to just nuke colorama from securesystemslib. IIRC @trishankatdatadog indicated some regret that it was added in the first place.

@jku
Copy link
Member

jku commented Oct 22, 2020

IMO we should at least test securesystemslib, which is basically a part of TUF in a separate package, with all its optional dependencies, because they were added for TUF.

Fair point, I guess this does make sense. Is this something you'd like fixed before upcoming release?

On the point of nuking colorama completely -- that probably makes sense? there should be a fairly high bar to adding dependencies to projects like this and coloramas usefulness does not seem to reach that bar...

@lukpueh
Copy link
Member

lukpueh commented Oct 22, 2020

Is this something you'd like fixed before upcoming release?

I think there is no fixing needed. Let's just leave this PR merged as is (I did add a note about it to the changelog) and remove colorama in securesystemslib rather soon, hopefully before they push an update that breaks tuf without us noticing, which I think is fairly unlikely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants