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

Jump to bottom

django-simple-captcha: init at 0.5.6 #64523

Closed
wants to merge 2 commits into from
Closed

django-simple-captcha: init at 0.5.6 #64523

wants to merge 2 commits into from

Conversation

MrMebelMan
Copy link
Contributor

Motivation for this change

Adding a missing Django package. It depends on django-ranged-response.

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 nix-review --run "nix-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.

@MrMebelMan MrMebelMan requested a review from FRidh as a code owner July 9, 2019 13:25
@risicle
Copy link
Contributor

risicle commented Jul 9, 2019

If you're adding a bunch of inter-related packages, it's probably a better idea to combine them all into a single PR.

lsix
lsix requested changes Jul 9, 2019
View reviewed changes
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

As already mentioned, you should include the commits of other PRs you depend on here.

Finally, I think the commit message should be pythonPackages.django-simple-captcha: init at 0.5.6 instead of django-simple-captcha: init at 0.5.6 (it is easier to follow git log that way)

pkgs/development/python-modules/django-simple-captcha/default.nix Outdated

buildPythonPackage rec {
pname = "django-simple-captcha";
version = "0.5.6";
Copy link
Member

Choose a reason for hiding this comment

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

There is a 0.5.11 version available, why not upgrade to it now (https://pypi.org/project/django-simple-captcha/)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll change it. The latest version doesn't work with some of the older django versions though. What is your policy on packaging multiple different versions (like in django's case)? Can I package both 0.5.6 and 0.5.11, making 0.5.11 the default one?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please see pg8000 and pg8000_1_12 as an example.

pkgs/development/python-modules/django-simple-captcha/default.nix Outdated
pname = "django-simple-captcha";
version = "0.5.6";

src = fetchurl {
Copy link
Member

Choose a reason for hiding this comment

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

Since the package is available on pypi, I think fetchPypi should be prefered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried PyPi, but there's no .tar.gz file available for 0.5.6 and I wasn't able to build from the .whl yet :(

pkgs/development/python-modules/django-simple-captcha/default.nix Outdated
sha256 = "02sbbsjlxa445hdfpxj4wmy1ynfj1vj0xblqhs7r05a2pbl3dzfn";
};

doCheck = false;
Copy link
Member

Choose a reason for hiding this comment

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

Could you add an explanation why the tests are disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no much to explain, I just wasn't able to run them :)
Updated the code as per this suggestion

Copy link
Member

Choose a reason for hiding this comment

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

We should try to make them work, what was the problem?

risicle
risicle reviewed Jul 9, 2019
View reviewed changes
pkgs/development/python-modules/django-simple-captcha/default.nix Outdated
sha256 = "02sbbsjlxa445hdfpxj4wmy1ynfj1vj0xblqhs7r05a2pbl3dzfn";
};

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. 

  checkInputs = [ testfixtures ];
  checkPhase = ''
    cd testproject
    ${python.interpreter} manage.py test captcha
  '';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, thanks!

risicle
risicle reviewed Jul 9, 2019
View reviewed changes
pkgs/development/python-modules/django-simple-captcha/default.nix Outdated

doCheck = false;

buildInputs = [ mock ];
Copy link
Contributor

@risicle risicle Jul 9, 2019 •
edited

Choose a reason for hiding this comment

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

Again, I can't see a need for this, and in fact having it obscures the fact that django-simple-captcha actually depends on six, which it's only seeing incidentally as it's brought in by this mock dependency - but of course it isn't propagated so it won't get picked up as a proper dependency of the nix package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for the explanation!

risicle
risicle reviewed Jul 9, 2019
View reviewed changes
pkgs/development/python-modules/django-simple-captcha/default.nix Outdated
doCheck = false;

buildInputs = [ mock ];
propagatedBuildInputs = [ django django-ranged-response pillow ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs six too.

@MrMebelMan
Copy link
Contributor Author

attached the django-ranged-response commit, bumped the django-simple-captcha version to 0.5.11, added tests and quoted the homepage URL. Now trying to figure out, how to build from wheel packages :P

This was referenced Jul 10, 2019
Closed
Merged
@risicle
Copy link
Contributor

risicle commented Jul 10, 2019

Don't try and build from wheel packages - I don't think it's possible. If there's a version you can't find a tarball for, try and get it using fetchFromGitHub instead.

@MrMebelMan
Copy link
Contributor Author

Don't try and build from wheel packages - I don't think it's possible. If there's a version you can't find a tarball for, try and get it using fetchFromGitHub instead.

done!

@mmahut
Copy link
Member

mmahut commented Aug 13, 2019

@GrahamcOfBorg eval

@schmittlauch
Copy link
Member

schmittlauch commented Dec 24, 2019 •
edited

@MrMebelMan Any news on this? I need django-simple-captcha myself and started packaging it, but you seem to already have made more progress than I did.

@schmittlauch
Copy link
Member

schmittlauch commented Dec 25, 2019 •
edited

@MrMebelMan Did you ever manage to get the tests to pass? For me they don't.

The trick for fetching the sources from PyPI is specifiying a custom extension as for whatever reason the sources are only available as zip.

I took the liberty to adapt your nix expression at schmittlauch@708d6be .
But as already written the test do not pass yet. We should join forces on tackling that issue.

@MrMebelMan
Copy link
Contributor Author

Sorry, @schmittlauch, I don't think my Nix-fu is good enough to fix it :(

Btw, your tests seem to pass!

running install tests
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
...s.............................
----------------------------------------------------------------------
Ran 33 tests in 0.597s

OK (skipped=1)
Destroying test database for alias 'default'..

@schmittlauch schmittlauch mentioned this pull request Dec 25, 2019
Merged
10 tasks
@schmittlauch
Copy link
Member

I created a superseding PR for this as #76515

@mmahut
Copy link
Member

mmahut commented Jan 22, 2020

Merged in #76515.

@mmahut mmahut closed this Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmahut mmahut mmahut left review comments

@risicle risicle risicle left review comments

@lsix lsix lsix requested changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MrMebelMan @risicle @mmahut @schmittlauch @lsix