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

Add flexmock, sqlalchemy-utils, pgpy, and py-gfm #50328

Closed
wants to merge 20 commits into from

Conversation

@bsima
Copy link

commented Nov 13, 2018

Motivation for this change

Trying to build core.sr.ht for NixOS and running into missing packages, figured I would add them upstream. I tested each one with nix-build -A pythonPackages.<pkgname> to make sure they built.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@bsima bsima requested a review from FRidh as a code owner Nov 13, 2018

@lsix
Copy link
Member

left a comment

Hi,

Thanks for the PR.

Could you add meta attributes for flexmock, PGPy and py-gfm ?

@bsima

This comment has been minimized.

Copy link
Author

commented Nov 14, 2018

@bsima

This comment has been minimized.

Copy link
Author

commented Nov 15, 2018

@dotlambda
Copy link
Member

left a comment

The requested changes should be done for the packages I didn't comment on as well.
You might want to read through https://nixos.org/nixpkgs/manual/#python again.

@bsima

This comment has been minimized.

Copy link
Author

commented Nov 16, 2018

@bsima

This comment has been minimized.

Copy link
Author

commented Nov 28, 2018

@dotlambda Anything I can do to help move this along?

@worldofpeace
Copy link
Member

left a comment

You don`t need to set any of these meta attributes

downloadPage = foo;
priority = 0;
broken = false;
updateWalker = true;

downloadPage could be useful but we already know where we can download python packages from (pypi.org).

Additionally you've set platforms to all. This is already set to automatically to


which should be sufficient.

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

@bsima
Could you also fixup these additional commits with an interactive rebase?
It would make reviewing commit by commit possible.

@bsima bsima force-pushed the bsima:python-packages branch from e698546 to a00aa73 Dec 4, 2018

@bsima

This comment has been minimized.

Copy link
Author

commented Dec 4, 2018

};

checkInputs = [ nose pytest ];

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member

Noticed the tests weren't being ran.

This should fix that:

Suggested change
checkPhase = ''
pytest tests/
'';

meta = with lib; {
homepage = https://flexmock.readthedocs.io/en/latest/;
description = "flexmock is a testing library for Python.";

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member

description should not have the name of said software in it or a period at the end.

https://nixos.org/nixpkgs/manual/#sec-standard-meta-attributes
Don’t include a period at the end. Don’t include newline characters. Capitalise the first character. For brevity, don’t repeat the name of package — just describe what it does.

Suggested change
description = "flexmock is a testing library for Python.";
description = "A testing library for Python that makes it easy to create mocks, stubs, and fakes";
meta = with lib; {
homepage = https://flexmock.readthedocs.io/en/latest/;
description = "flexmock is a testing library for Python.";
longDescription = ''

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member

This description isn't long so we don't need it.

}:

buildPythonPackage rec {
pname = "PGPy";

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member

I think we're normalizing pname now so this should be

Suggested change
pname = "PGPy";
pname = "pgpy";
version = "0.4.3";

src = fetchPypi {
inherit pname version;

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member

Changing the pname will most likely make this not work here.
So we might need:

Suggested change
inherit pname version;
pname = "PGPy";
inherit version;

meta = with lib; {
homepage = https://github.com/SecurityInnovation/PGPy;
description = "Pretty Good Privacy for Python 2 and 3.";

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member
Suggested change
description = "Pretty Good Privacy for Python 2 and 3.";
description = "Pretty Good Privacy for Python 2 and 3";
sha256 = "04412dddd6882ac0c5d5daf4326c28d481421851a68e25e7ac8e06cc9dc2b902";
};

propogatedBuildInputs = [ cryptography pyasn1 six singledispatch ];

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member

See https://github.com/SecurityInnovation/PGPy/blob/f1c3d68e32c334f5aa14c34580925e97f17f4fde/setup.py#L36

only depend on enum34 if Python is older than 3.4

We can constrain the addition of that with

Suggested change
propogatedBuildInputs = [ cryptography pyasn1 six singledispatch ];
propogatedBuildInputs = [ cryptography pyasn1 six singledispatch ] ++ lib.optional (pythonOlder "3.4") enum34;

meta = with lib; {
homepage = https://github.com/zopieux/py-gfm;
description = "GitHub-Flavored Markdown as an extension to the Python Markdown library.";

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member
Suggested change
description = "GitHub-Flavored Markdown as an extension to the Python Markdown library.";
description = "GitHub-Flavored Markdown as an extension to the Python Markdown library";
meta = with lib; {
homepage = https://github.com/zopieux/py-gfm;
description = "GitHub-Flavored Markdown as an extension to the Python Markdown library.";
longDescription = ''

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member

You can have the long description if you want, but it's just a second sentence.

sha256 = "ef6750c579d26651cfd23968258b604228fd71b2a4e1f71dea3bea289e01377e";
};

buildInputs = [ markdown ];

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member
Suggested change
buildInputs = [ markdown ];
propagatedBuildInputs = [ markdown ];
}:

buildPythonPackage rec {
pname = "SQLAlchemy-Utils";

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member

I think we're normalizing pname now so this should be

Suggested change
pname = "SQLAlchemy-Utils";
pname = "sqlalchemy-utils";
version = "0.33.6";

src = fetchPypi {
inherit pname version;

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member

Changing the pname will most likely make this not work here.
So we might need:

Suggested change
inherit pname version;
pname = "SQLAlchemy-Utils";
inherit version;
sha256 = "45ab41c90bfb8dd676e83179be3088b3f2d64b613e3b590187163dd941c22d4c";
};
checkInputs = [ pytest flexmock mock python-dateutil pytz ];
buildInputs = [ six sqlalchemy ];

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member
Suggested change
buildInputs = [ six sqlalchemy ];
propagatedBuildInputs = [ six sqlalchemy ];

meta = with lib; {
homepage = https://github.com/kvesteri/sqlalchemy-utils;
description = "Various utility functions and datatypes for SQLAlchemy.";

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member
Suggested change
description = "Various utility functions and datatypes for SQLAlchemy.";
description = "Various utility functions and datatypes for SQLAlchemy";
sha256 = "b388399caeeccdc145f06fd0d2665eeecc545385c60b55c282a15a022215af80";
};

buildInputs = [ cryptography ecdsa ];

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member
Suggested change
buildInputs = [ cryptography ecdsa ];
propagatedBuildInputs = [ cryptography ecdsa ];
buildInputs = [ cryptography ecdsa ];

# For some reason nix can't find the "tests" module.
doCheck = false;

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member

Seems something is not right with the pypi tarball.
Everything works fine when I use the github repo.

src = fetchFromGitHub {
    owner = "ojarva";
    repo = "python-sshpubkeys";
    rev = "v${version}";
    sha256 = "0000000000000000000000000000000000000000000000000000"; # change this
};

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Dec 4, 2018

Member

Please also add a comment above it explaining why we have to use github.

@worldofpeace
Copy link
Member

left a comment

Additionally tests need aren't running or are missing checkInputs for:

  • py-gfm
  • pgpy

@eadwu eadwu referenced this pull request Jan 21, 2019

Open

sourcehut: init #54425

3 of 10 tasks complete
@bsima

This comment has been minimized.

Copy link
Author

commented Jan 29, 2019

Closing in lieu of #54425 - much more progress and better code in that one :)

@bsima bsima closed this Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.