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

update colorama and related #26311

Merged
merged 4 commits into from Jun 3, 2017
Merged

update colorama and related #26311

merged 4 commits into from Jun 3, 2017

Conversation

rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Jun 1, 2017

Motivation for this change
Things done
  • Tested using sandboxing
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip" (treq is failing but seems unrelated)
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

FRidh
FRidh requested changes Jun 2, 2017
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Please move all expressions to separate files.

pkgs/top-level/python-packages.nix Outdated

colorama = buildPythonPackage rec {
name = "colorama-${version}";
colorama37 = self.colorama.overrideAttrs (old: rec {
Copy link
Member

Choose a reason for hiding this comment

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

  • only a single version of a package in pythonPackages. If you really need another version, include it via a let ... in expression. Only if multiple packages need this extra version, we introduce it globally.
  • when appending a version number, its supposed to be colorama_3_7 to avoid ambiguation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I could probably remove the check since the release it's just bugfixes but the developer of awscli says colorama had been breaking the program before.

Copy link
Member

Choose a reason for hiding this comment

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

Changes in colorama have indeed caused breakage in the past. Let's leave such an optimization to the awscli maintainer.

FRidh
FRidh approved these changes Jun 2, 2017
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

haven't tested it but looks good.

@Mic92
Copy link
Member

Mic92 commented Jun 3, 2017

When I starting tmuxp, I get the following error message:

Traceback (most recent call last):
  File "/nix/store/zphh0zdij5mjv1hccmd2lp7x746lkl5n-tmuxp-1.3.1/bin/.tmuxp-wrapped", line 8, in <module>
    from tmuxp import cli
  File "/nix/store/zphh0zdij5mjv1hccmd2lp7x746lkl5n-tmuxp-1.3.1/lib/python2.7/site-packages/tmuxp/__init__.py", line 20, in <module>
    from . import config, util, cli
  File "/nix/store/zphh0zdij5mjv1hccmd2lp7x746lkl5n-tmuxp-1.3.1/lib/python2.7/site-packages/tmuxp/cli.py", line 17, in <module>
    from libtmux.common import has_minimum_version, which
ImportError: cannot import name has_minimum_version

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jun 3, 2017

libtmux was too old. For one time the requirements.txt constraint was necessary.

@Mic92 Mic92 merged commit 4c03492 into NixOS:master Jun 3, 2017
@rnhmjoj rnhmjoj deleted the colorama branch September 12, 2017 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rnhmjoj @Mic92 @FRidh