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.python-openid: init at 2.2.5 #38788

Closed
wants to merge 1 commit into from

Conversation

timokau
Copy link
Member

@timokau timokau commented Apr 11, 2018

Motivation for this change

Package python-openid.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
  • Fits CONTRIBUTING.md.

description = "Adds openid support to flask applications";
license = licenses.bsd2;
maintainers = with maintainers; [ timokau ];
homepage = https://pythonhosted.org/Flask-OpenID/;
Copy link
Member

Choose a reason for hiding this comment

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

wrong homepage

'';

meta = with stdenv.lib; {
description = "Adds openid support to flask applications";
Copy link
Member

Choose a reason for hiding this comment

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

wrong description


meta = with stdenv.lib; {
description = "Adds openid support to flask applications";
license = licenses.bsd2;
Copy link
Member

Choose a reason for hiding this comment

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

licenses.asl20

propagatedBuildInputs = [
django
twill
pycrypto
Copy link
Member

Choose a reason for hiding this comment

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

Are these really runtime dependencies? I don't see any install_requires in https://github.com/openid/python-openid/blob/2.2.5/setup.py.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Then they probably belong in checkInputs

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.

This should be disabled for Python 3: https://github.com/openid/python-openid#requirements

I'm against packaging this. See openid/python-openid#66 and openid/python-openid#67. Since the last commit is from 2014, it doesn't seem like this will be fixed at all. It should at least be marked as insecure.

@timokau
Copy link
Member Author

timokau commented Apr 13, 2018

The first seems to have a patch available that we could fetch (and its only DoS anyways). The second seems to be resolvable by adding a pycurl dependency.

@dotlambda
Copy link
Member

Yes, these two might be fixable. But it's generally a bad idea to use security-related software that hasn't been updated in 4 years. There are probably many more undiscovered issues.

@timokau
Copy link
Member Author

timokau commented Apr 13, 2018

But it's generally a bad idea to use security-related software that hasn't been updated in 4 years

Thats true, but imo not in the control of a package manager. I am packaging this because sagenb (jupyter notebook) depends on flask-openid which depends on this. In the case of a jupyter notebook, the bad security might be acceptable. But in any case, wheather or not to use this is the decision of the upstream developers (sagenb in this case).

@dotlambda
Copy link
Member

What does @FRidh think?

@dotlambda dotlambda self-assigned this Apr 14, 2018
@FRidh
Copy link
Member

FRidh commented Apr 14, 2018

The last commit is 4 years old, and the last release 8 years. That's not going in.

@timokau not everything one needs, needs to be upstreamed to Nixpkgs. In this case you can keep the expressions for yourself or elsewhere.

@FRidh FRidh closed this Apr 14, 2018
@timokau
Copy link
Member Author

timokau commented Apr 15, 2018

Thats a shame. Does that mean I can't upstream sage at all or can I just include python-openid as a private dependency that is not exposed?

not everything one needs, needs to be upstreamed to Nixpkgs.

Of course not, but its usually better for everybody when it is.

@FRidh
Copy link
Member

FRidh commented Apr 15, 2018

Investigate whether this dependency is really needed. openid is just an authentication method so it's unlikely it is a mandatory dependency.

Of course not, but its usually better for everybody when it is.

Indeed, usually. Large pieces of software with many dependencies (sage and gitlab but also say chromium) require quite the dedication to make and keep it working. I would love to see these in Nixpkgs (chromium is, gitlab also but is causing trouble), but they require dedicated maintainers, and typically more than one as well. If there is a working implementation, stable over some time, then it can absolutely go in. Until then I suggest to keep it in an overlay and find collaborators for it.

@timokau
Copy link
Member Author

timokau commented Apr 15, 2018

I would love to see these in Nixpkgs (chromium is, gitlab also but is causing trouble), but they require dedicated maintainers, and typically more than one as well.
If there is a working implementation, stable over some time, then it can absolutely go in. Until then I suggest to keep it in an overlay and find collaborators for it.

Since I resurrected the sage build (current nixpkgs sage rebuilds all its dependencies in isolation), some people already helped me debug it. Among them (non-comprehensive) @7c6f434c, @dtzWill, @vcunat.

Other people reported issues for it:

While that doesnt proof that theres sufficient interest to sustain it, I think its worth to give it a shot. I don't think maintaining an overlay would work as well.

@7c6f434c
Copy link
Member

@FRidh that claim about «more than one» is an idealistic dream which doesn't correspond to what actually happens with TeXLive, LibreOffice etc. Using it as a gatekeeping criteria seems a bit too much to me.

@timokau has already managed to create a working Sage package, but Sage bundles everything and that creates quite a bit of pain so the goal is to make it behave by using system-wide packages for dependencies. «Keep it local» in this case will end up meaning a sage/default.nix-local package definition.

Also, I think that judging whether the software — that still works — is useful by whether it keeps changing all the time is just making the bit rot problem worse.

I do agree with your policy of marking things as broken if problems arise and are not fixed quickly by somebody, but in case of a Sage dependency it is quite likely that once there are actual problems there will be either a patch or a Sage version not needing the package (in this case dropping sounds completely natural)

@FRidh
Copy link
Member

FRidh commented Apr 18, 2018

that claim about «more than one» is an idealistic dream which doesn't correspond to what actually happens with TeXLive, LibreOffice etc. Using it as a gatekeeping criteria seems a bit too much to me.

I don't know if you can compare them. With the information that's been given to me in these PR's we deal here with apparently quite some outdated and unmaintained dependencies.

I'm not against having sage or sagenb in. Before going on, which one are we now really talking about Since I've seen PR's related to sagenb, and then the discussion shifted to sage.

has already managed to create a working Sage package

Great!

I do agree with your policy of marking things as broken if problems arise and are not fixed quickly by somebody, but in case of a Sage dependency it is quite likely that once there are actual problems there will be either a patch or a Sage version not needing the package (in this case dropping sounds completely natural)

See the linked thread sagemath/sagenb#440. They're not moving that fast.

@7c6f434c
Copy link
Member

I hope «more than one» was about the Sage undertaking as a whole. This PR is just about one of the thousand paper cuts about building Sage (even if the build works somehow).

Sage is not moving fast about doing something about unmaintained but still working dependencies, and I support lack of hurry in this respect. I mean, there should be some incentive to use code that has a chance to work for more than three months after release. I hope that if a dependency becomes actually hard to build after some transitive dependency update, Sage will have either a patch or a way to do without this dependency faster than if the dependency works and there is no reason to expect it to break.

@timokau timokau deleted the python-openid-init branch May 24, 2018 22:10
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