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
Contributor

@bsima bsima 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.

Copy link
Member

@lsix lsix left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for the PR.

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

@bsima
Copy link
Contributor Author

bsima commented Nov 14, 2018 via email

@bsima
Copy link
Contributor Author

bsima commented Nov 15, 2018 via email

Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

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.

pkgs/development/python-modules/flexmock/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pgpy/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pgpy/default.nix Outdated Show resolved Hide resolved
@bsima
Copy link
Contributor Author

bsima commented Nov 16, 2018 via email

@bsima
Copy link
Contributor Author

bsima commented Nov 28, 2018

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

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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

@bsima
Copy link
Contributor Author

bsima commented Dec 4, 2018 via email

};

checkInputs = [ nose pytest ];

Copy link
Contributor

Choose a reason for hiding this comment

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

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.";
Copy link
Contributor

Choose a reason for hiding this comment

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

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 = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}:

buildPythonPackage rec {
pname = "PGPy";
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.";
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

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.";
Copy link
Contributor

Choose a reason for hiding this comment

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

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 = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

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

sha256 = "ef6750c579d26651cfd23968258b604228fd71b2a4e1f71dea3bea289e01377e";
};

buildInputs = [ markdown ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
buildInputs = [ markdown ];
propagatedBuildInputs = [ markdown ];

}:

buildPythonPackage rec {
pname = "SQLAlchemy-Utils";
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

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.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description = "Various utility functions and datatypes for SQLAlchemy.";
description = "Various utility functions and datatypes for SQLAlchemy";

sha256 = "b388399caeeccdc145f06fd0d2665eeecc545385c60b55c282a15a022215af80";
};

buildInputs = [ cryptography ecdsa ];
Copy link
Contributor

Choose a reason for hiding this comment

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

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

buildInputs = [ cryptography ecdsa ];

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

Choose a reason for hiding this comment

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

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
};

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

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

  • py-gfm
  • pgpy

@eadwu eadwu mentioned this pull request Jan 21, 2019
10 tasks
@bsima
Copy link
Contributor Author

bsima 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants