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

python2Packages.requests: Unbreak #115048

Merged
merged 1 commit into from Mar 4, 2021
Merged

Conversation

adisbladis
Copy link
Member

Motivation for this change

#114768 (comment)

@SuperSandro2000
In exactly the same number of lines of code you code have fixed the problem...

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@@ -33,6 +33,9 @@ buildPythonPackage rec {
pytestCheckHook
];

# AttributeError: 'KeywordMapping' object has no attribute 'get'
doCheck = ! isPy27;
Copy link
Member

Choose a reason for hiding this comment

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

That's not a solution either.
The failing test probably points to some dependency not being compatible with Python 2.

Copy link
Member Author

@adisbladis adisbladis Mar 4, 2021

Choose a reason for hiding this comment

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

That error is raised in pytest, so it's not an issue for requests itself.

Copy link
Member

Choose a reason for hiding this comment

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

I know, but means that one of the dependencies (maybe pytest) is not compatible with Python 2.

Copy link
Member

Choose a reason for hiding this comment

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

If upstream dropped python2 support we should add the latest compatible version for python 2 to nixpkgs as long as it is needed to not have any surprises or strange breakages.

Copy link
Member

Choose a reason for hiding this comment

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

Requests still supports it and so does the version of pytest we package for Python 2.

@adisbladis adisbladis merged commit 893a78f into NixOS:master Mar 4, 2021
@SuperSandro2000
Copy link
Member

In exactly the same number of lines of code you code have fixed the problem...

I just marked it broken so that I do not try to build it to often and give people a false sense that they broke something. The problem wasn't so trivial for me that I could just fix it and Python 2 is also not my top priority.

@adisbladis
Copy link
Member Author

adisbladis commented Mar 4, 2021

I just marked it broken so that I do not try to build it to often and give people a false sense that they broke something. The problem wasn't so trivial for me that I could just fix it and Python 2 is also not my top priority.

You could have opened an issue or a separate PR where a discussion could be had..
If you are not qualified to work on something then involve someone who is.

Stop defending your bad actions. Own up to them and learn from your mistakes.

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

Successfully merging this pull request may close these issues.

None yet

3 participants