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

Convert colorama into an optional dependency #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VaultVulp
Copy link

@VaultVulp VaultVulp commented May 6, 2020

I propose an update to the list of dependecies required to use icecream.

Right now, we have to install colorama on all platforms, despite the fact that ic will run properly on some operating systems even without colorama presented.

As stated, I suggest:

  • Converting colorama into an optional dependecy;
  • Wrapping colorama import and initialization in a platform-dependent if-block;
  • Some refactoring to the icecream.py;

* Update install_requires in setup.py
* Add platform check before colorama import
* Remove Windows specific context manager
# API calls. This code does nothing on non-Windows systems.
colorama.init()
yield
colorama.deinit()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could easily write this to still use a context manager and deinit after each print. Any reason not to?

Copy link
Author

@VaultVulp VaultVulp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for you interest to my PR, @alexmojaki .

I spent a little time tweaking with the code base, trying to minimize code change, but I wasn't able to find a readable solution.

If we leave this contex manager in place, we should move conditional import into this context manager, thus we will try to import colorama on each call of ic. And import statement is a costly opertaion.

I've tried to move context manager into another module (windows_support), to be able to import colorama on a module level. But in that case we have to use conditional function definition such as:

if sys.platform == 'win32':
    from .windows_support import supportTerminalColorsInWindows

    def colorizedStderrPrint(s):
        colored = colorize(s)
        with supportTerminalColorsInWindows():
            stderrPrint(colored)
else:
    def colorizedStderrPrint(s):
        colored = colorize(s)
        stderrPrint(colored)

In my opinion it comlicates both icecream.py and entrire package. So I had to throw away this idea.

After that I've read a little bit of colorama's documentation to find out the reason to deinit colorama, but wasn't succesfful. So I wasn't sure why do we need to call init and deinit in such manner.

After a while, I've rewriten my proposition from ground up, and created this PR.

If you can help me understand the reason behind this context manager, I will try to rewrite this PR again.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See if you can do without init() and deinit() entirely using colorama.AnsiToWin32.

Copy link
Author

@VaultVulp VaultVulp May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can write:

if sys.platform.startswith('win'):
    # We will import colorama only on a specific subset of OS's, and wrap sys.stderr into colorama's stream wrapper
    import colorama
    colorama.init(wrap=False)
    output_stream = colorama.AnsiToWin32(sys.stderr).stream
else:
    output_stream = sys.stderr

...

def stderrPrint(*args):
    print(*args, file=output_stream)

Sadly, in this case we have to rewrite captureStandardStreams context manager in test_icecream.py, for it tries to replace stderr with a fake stream. I tride to monkey-patch newly added module level attribute, but without any luck.

@gruns
Copy link
Owner

gruns commented May 9, 2020

I have no problem with an optional colorama dependency, per se. But let's back up quickly to make sure we're all on the same page and tackling the right problem.

Why would you like to move colorama to an optional dependency? For example

  1. Faster import speeds? (fewer imports = faster import speed)
  2. Smaller install footprint? (ie faster install speed, smaller size on disk)
  3. You want color disabled by default?

And/or other reason(s) entirely.

@VaultVulp
Copy link
Author

Thanks for your attention, @gruns!

I'm using pipenv as the main tool for dependency management.

It allows me to set platform specific dependencies.
For example:

uvloop = {version = "==0.14.0", markers = "sys_platform != 'win32' and implementation_name == 'cpython'"}
colorama = {version = "==0.4.3", markers = "sys_platform == 'win32' and implementation_name == 'cpython'"}

As you know less dependecies means less version conflicts, less security issues etc. So usually I prefer to not install colorama on windows.

pipenv also supports a lock-file - automatically generated file that helps with the validation of a checksum of a package before installing it.
It's a very useful security feature, but pipenv always pushes colorama's platform marker into lock-file, if me or one of my teammates generates this lock-file on Windows.
And after that, we cannot run our project reliably in a non-windows environment, since colorama won't be presented in python's environment, but icecream will be.

Maybe this concrete issue (pipenv + icecream + colorama) should be solved through pipenv's repo, but their release cycle is unbelievably long. So I've started with icream at first.

@gruns
Copy link
Owner

gruns commented May 14, 2020

Thank you for expounding; I understand your predicament much better now.

It's a very useful security feature, but pipenv always pushes colorama's platform marker into lock-file This is the money line.

From Googling, we're not the first to be bitten by this: pypa/pipenv#3902.

  1. Does the --keep-outdated pipenv flag resolve the problem for you -- either with pipenv 2018.11.26 (the last stable release) or master? I unfortunately don't have a Windows machine to test on.

  2. Since pipenv's release cycle is glacial, nigh deceased, can this be fixed from colorama's end?

  3. Does Poetry (https://python-poetry.org/) suffer the same problem here?

If none of the above are satisfactory, perhaps it's time indeed to turn colorama into an optional icecream dependency.

@VaultVulp
Copy link
Author

Sorry, @gruns, was a little bit busy with my work.

Answering your questions:

  1. I've tried --keep-outdate, but without any luck - problem was still present.
  2. I doubt colorama can solve this issue. Colorama is a Windows specific package, and it makes sense for them to not be installed on any other platform.
  3. It might be a solution, but I'm an active pipenv user and it will be nuisance to switch to another package manager, and to enforce a new package manager tool for my dev team.

@VaultVulp
Copy link
Author

pipenv published a new release recently. I've checked the described problem, but it's still present, sadly - if i'm trying to generate a lock-file on Windows platform pipenv converts colorama a Windows specific dependency.

@gruns
Copy link
Owner

gruns commented Jun 30, 2020

Marking colorama as platform_system=='Windows' in setup.py should do the trick nicely and avoid runtime if sys.platform.startswith('win'): checks.

One problem: I don't have ready access to a Window machine to test. @VaultVulp Would you be able to graciously continue to assist and test on Windows?

@VaultVulp
Copy link
Author

VaultVulp commented Jun 30, 2020

For sure, @gruns, I'll help you.

But if we do not move colorama's import under an if-case, Python will try to import colorama and fail with the ModuleNotFoundError.

@VaultVulp
Copy link
Author

Hi, folks! You asked for my help @gruns , can I help you with anything?

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.

None yet

3 participants