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.mezzanine: fixes #19989 #20152

Merged
merged 1 commit into from
Nov 30, 2016
Merged

Conversation

igsha
Copy link
Contributor

@igsha igsha commented Nov 4, 2016

Motivation for this change

mezzanine depends on Django-1.6 that does not support python>3.3. But it also depends on filebrowser_safe that depens on self.django that is actually a Django-1.10. I've explicitly overrided django for dependent filebrowser_safe to get a correct installation error.

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
    • OS X
    • 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.

Tests:

[15:01] igor@nixos-pc nixpkgs (master *) $ nix-shell -I nixpkgs=~/nixpkgs -p python33Packages.mezzanine
[15:01] igor@nixos-pc nixpkgs (master *) [nix-shell] % python3
Python 3.3.6 (default, Oct 12 2014, 07:03:57) 
[GCC 5.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import mezzanine
>>> from mezzanine import utils
>>> 
[15:02] igor@nixos-pc nixpkgs (master *) [nix-shell] % exit
[15:02] igor@nixos-pc nixpkgs (master *) $ nix-shell -I nixpkgs=~/nixpkgs -p python34Packages.mezzanine
error: Django-1.6.11 not supported for interpreter python3.4m
(use ‘--show-trace’ to show detailed location information)
[15:02] igor@nixos-pc nixpkgs (master *) ✗ (1) $ nix-shell -I nixpkgs=~/nixpkgs -p python35Packages.mezzanine
error: Django-1.6.11 not supported for interpreter python3.5m
(use ‘--show-trace’ to show detailed location information)
[15:02] igor@nixos-pc nixpkgs (master *) ✗ (1) $ 

Sorry, something went wrong.

@igsha
Copy link
Contributor Author

igsha commented Nov 4, 2016

cc @lsix

@FRidh
Copy link
Member

FRidh commented Nov 4, 2016

#19989

@FRidh
Copy link
Member

FRidh commented Nov 4, 2016

Packages shouldn't depend on a specific version by directly referring to the attribute. What I mean is, its not good to have django_1_6 as a buildInput. Instead, use the approach as is done by lti, i.e., override self.

let 
  self' = (self.override {self = self';}) // {django = django_1_6;};
in .....

 propagatedBuildInputs = with self' [ django ... ];

@igsha
Copy link
Contributor Author

igsha commented Nov 4, 2016

Perfectly, it works: it has the same behaviour for python35, python34 and python33. Yes, it is better to override self instead of each package.

@FRidh
Copy link
Member

FRidh commented Nov 4, 2016

even so, I am not so fond of having this. What you easily get now is that you include this package, and then another package that also depends on django, and suddenly you have two versions of django in your closure...oops!

Instead, the user should override the package set and select the correct Django.

@igsha
Copy link
Contributor Author

igsha commented Nov 5, 2016

@FRidh, I've updated MR. Is it ready to be merged?

@FRidh
Copy link
Member

FRidh commented Nov 7, 2016

While I am not fond of this solution, it is okay. But, before I merge, is there any reason not to update mezzanine to the latest available version?

@FRidh
Copy link
Member

FRidh commented Nov 7, 2016

Because, unless some other Nixpkgs package needs it, we do not keep older versions.

@igsha
Copy link
Contributor Author

igsha commented Nov 8, 2016

Yes, it makes sense. But I need to update filebrowser_safe, grappelli_safe and add django-contrib-comments package. Can I do it in this MR or split them?

@FRidh
Copy link
Member

FRidh commented Nov 8, 2016

do you need all packages, including this mezzanine, at the latest version, or not? If not I suggest you use pypi2nix

@igsha
Copy link
Contributor Author

igsha commented Nov 8, 2016

No, I personaly don't need the latest version of mezzanine. I just want the mezzanine to be compiled because I can't move forward with pyjwt and oathlib packages that are required for conan.

@FRidh
Copy link
Member

FRidh commented Nov 8, 2016

ohh, ok. Since the mezzanine version we have is so old and because it is not worked you can add meta.broken = true;. Leave it to a user of mezzanine to fix it.

@lsix
Copy link
Member

lsix commented Nov 8, 2016

Seems fair to me to mark it as broken until someone proceed with the updates.

@FRidh FRidh merged commit c517718 into NixOS:master Nov 30, 2016
@igsha igsha deleted the fix-mezzanine branch November 18, 2021 18:51
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