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

pythonPackages.future: 0.15.2 -> 0.16.0 #26220

Merged
merged 1 commit into from
Jun 1, 2017

Conversation

erictapen
Copy link
Member

Motivation for this change

Needed a newer version for another package.

Things done

Bumped version and used fetchFromGitHub insted of fetchurl.

  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • 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"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@erictapen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @FRidh to be a potential reviewer.

@FRidh
Copy link
Member

FRidh commented May 30, 2017

Can you move this expression to pkgs/development/python-modules/future/default.nix.

and used fetchFromGitHub insted of fetchurl.

Why?

@erictapen
Copy link
Member Author

Can you move this expression to pkgs/development/python-modules/future/default.nix.

Will edit this asap. Missed that policy.

Why?

I felt like fetchFromGitHub would fit better, as it is more specific. Please correct me if you think that it is not good practice.

@FRidh
Copy link
Member

FRidh commented May 30, 2017

@erictapen with fetchPypi we can update the version and hash automatically (see maintainers/scripts/update-python-libraries)

@erictapen
Copy link
Member Author

erictapen commented May 30, 2017

That sounds reasonable!

Moved the expression to ./pkgs/development/python-modules/future/default.nix and used fetchPypi.

Please note, that this is my first contribution besides a simple version bump. I do not really understand what I've done, I just changed things until my package compiled (and worked).

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 note, that this is my first contribution besides a simple version bump. I do not really understand what I've done, I just changed things until my package compiled (and worked).

You've made a good start. Just these two edits and then it should be ok to go in. Before, we called the function buildPythonPackage with a set of arguments directly in python-packages.nix. Now, we moved that expression to a separate file. That file is a function, returning our derivation. So, we call a function with a set of arguments and that function calls buildPythonPackage with a set of arguments.

@@ -0,0 +1,39 @@
{ lib
, pkgs
Copy link
Member

Choose a reason for hiding this comment

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

don't pass in the whole package set, instead, use individual packages

sha256 = "1nzy1k4m9966sikp0qka7lirh8sqrsyainyf8rk97db7nwdfv773";
};

propagatedBuildInputs = lib.optionals isPy26 (with pkgs.pythonPackages; [ importlib argparse ]);
Copy link
Member

Choose a reason for hiding this comment

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

We are not going to use the package set, but we're going to pass in the individual requirements. Therefore, in the top, you can add the parameters importlib and argparse and remove here with pkgs.pythonPackages;

Also moved the expression from python-packages.nix to ./pkgs/development/python-modules/future/default.nix due to discussion in NixOS#26220
Used fetchPypi insted of fetchurl.
@erictapen
Copy link
Member Author

Thanks alot. Did you mean this?

@FRidh
Copy link
Member

FRidh commented May 30, 2017

Yep!

cc maintainer @prikhi

@FRidh FRidh merged commit 84ce0d8 into NixOS:master Jun 1, 2017
@erictapen erictapen deleted the python-future-bump branch June 1, 2017 21:30
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