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.bcrypt: use 3.1.x for python older than 3.6 #97996

Merged
merged 2 commits into from Sep 15, 2020

Conversation

mvnetbiz
Copy link
Contributor

Motivation for this change

pythonPackages.bcrypt was disabled for python older than 3.6: 87371b8

We still have some applications that cannot be upgraded from python 2 that require python-bcrypt
mysql-workbench, syncthing-gtk: #97642

ZHF: #97479

Things done

Added back pythonPackages.bcrypt 3.1.7 only for python older than 3.6

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

@drewrisinger
Copy link
Contributor

@mvnetbiz the standard way of doing this is overriding the package set to create a custom python installation with the version that you want. See for example https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/prospector/default.nix.

That is, for mysql_workbench.nix, you add something like

let
  py2 = python2.override {
    packageOverrides = self: super:
      bcrypt = super.bcrypt.overridePythonAttrs(oldAttrs: rec {
        version = "3.1.X";
        src = oldAttrs.src.override {
          inherit version;
          sha256 = "...";
        };
        disabled = false;
      };
    });
  };
in
  ...

@mvnetbiz
Copy link
Contributor Author

Since it is deprecated it seems like it would make sense to keep the definition in the package that requires it, so it can be removed once it isn't needed any more, but this is used by at least 2 used packages, also the expression might deviate significantly from the new version, would that end up being too verbose keeping in a per-use override? Would it be better to add a separate pythonPackages.bcrypt_3_1, and use that in these packages?

@drewrisinger
Copy link
Contributor

Since it is deprecated it seems like it would make sense to keep the definition in the package that requires it, so it can be removed once it isn't needed any more, but this is used by at least 2 used packages, also the expression might deviate significantly from the new version, would that end up being too verbose keeping in a per-use override? Would it be better to add a separate pythonPackages.bcrypt_3_1, and use that in these packages?

I took about 10 mins and looked at usages of bcrypt throughout nixpkgs. In the applications that need it, bcrypt is always overridden to a version ~3.1.7 (e.g. home-assistant). I think this is probably the path that you should take. I wouldn't worry about long derivation overrides, most seem to be pretty short.

pinging maintainer of bcrypt @domenkozar

@mvnetbiz
Copy link
Contributor Author

IMHO, in that example, it is bcrypt's reverse dependency (home-assistant) breaking compatibility with it, so it makes sense to override it there. The reason for this patch is that bcrypt itself, not it's user, is dropping compatibility with python < 3.6, so anything requiring bcrypt on python2 will have to use this version, so I thought it made sense to have a more general patch. I can make the PRs for mysql-workbench, syncthing-gtk, (poretools?) if this gets closed.

@mvnetbiz
Copy link
Contributor Author

There are many modules defined this way in pkgs/top-level/python-packages.nix, at least the first 4 I looked at result from the same upstream dropping of old python version, this doesn't seem too atypical.

@jonringer
Copy link
Contributor

@drewrisinger that's only if it's a python application, if he still needs python2 to work, this is the correct method. We do it for packages such as numpy.

@jonringer
Copy link
Contributor

jonringer commented Sep 15, 2020

also, please disable python2Packages.trytond:

  Processing ./trytond-5.6.5-py2-none-any.whl
  ERROR: Package 'trytond' requires a different Python: 2.7.18 not in '>=3.5'
https://github.com/NixOS/nixpkgs/pull/97996
3 packages failed to build:
python27Packages.junos-eznc python27Packages.trackpy python27Packages.trytond

@mvnetbiz
Copy link
Contributor Author

mvnetbiz commented Sep 15, 2020

Should that be in the same PR? It's not caused by this branch, it fails to build on master too, just with:

error: bcrypt-3.2.0 not supported for interpreter python2.7
(use '--show-trace' to show detailed location information)

NVM I guess that makes sense if it's bcrypt related

Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

  • A few unnecessary things in diff
  • Commits LGTM
  • Partial build tested: nix-review -p python27Packages.bcrypt -p mysql-workbench
  • Maybe add disabled to python-modules/bcrypt/default.nix to be explicit in the file about where it's supported??
https://github.com/NixOS/nixpkgs/pull/97996
2 packages built:
mysql-workbench python27Packages.bcrypt

pkgs/development/python-modules/bcrypt/3_1.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/bcrypt/3_1.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/bcrypt/3_1.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/bcrypt/3_1.nix Outdated Show resolved Hide resolved
@mvnetbiz
Copy link
Contributor Author

@drewrisinger, thanks for the comments. python-modules/bcrypt/default.nix already has disabled from commit referenced in PR body

@drewrisinger
Copy link
Contributor

Looks like enabling this breaks python27Packages.[junos-eznc, trackpy] b/c it's semi-adding a new package.

https://github.com/NixOS/nixpkgs/pull/97996
2 packages failed to build:
python27Packages.junos-eznc python27Packages.trackpy

34 packages built:
mysql-workbench poretools python27Packages.Fabric python27Packages.ansible python27Packages.awkward python27Packages.bcrypt python27Packages.bkcharts python27Packages.docker python27Packages.drms python27Packages.duckdb python27Packages.flammkuchen python27Packages.flask-bcrypt python27Packages.imbalanced-learn python27Packages.ldaptor python27Packages.mapsplotlib python27Packages.moto python27Packages.ncclient python27Packages.pandas python27Packages.paramiko python27Packages.passlib python27Packages.privacyidea-ldap-proxy python27Packages.pwntools python27Packages.pysftp python27Packages.pytest-ansible python27Packages.robotframework-sshlibrary python27Packages.scp python27Packages.seaborn python27Packages.sshtunnel python27Packages.uproot python27Packages.uproot-methods python27Packages.vega_datasets python27Packages.vidstab python27Packages.yfinance syncthing-gtk

@mvnetbiz
Copy link
Contributor Author

trackpy is marked broken, and I don't think this broke junos-eznc, the derivation just couldn't evaluate previously, with error: bcrypt-3.2.0 not supported for interpreter python2.7. It is broken for some other reason apparently.

Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

  • Diff LGTM
  • Commits LGTM
  • Builds via nix-review (mostly, see above comments). Failing packages should be addressed in follow-up ZHF PR.

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

diff LGTM

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