Skip to content

pythonPackages.b2sdk: 1.6.0 -> 1.7.0#122138

Closed
uri-canva wants to merge 1 commit intoNixOS:masterfrom
uri-canva:b2sdk
Closed

pythonPackages.b2sdk: 1.6.0 -> 1.7.0#122138
uri-canva wants to merge 1 commit intoNixOS:masterfrom
uri-canva:b2sdk

Conversation

@uri-canva
Copy link
Contributor

@uri-canva uri-canva commented May 8, 2021

Motivation for this change

Upgrade b2sdk.
Fix backblaze-b2 on darwin.
Failure: https://hydra.nixos.org/build/142494474
ZHF: #122042

Things done
  • 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.

@uri-canva uri-canva requested review from FRidh and jonringer as code owners May 8, 2021 00:42
@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label May 8, 2021
@uri-canva uri-canva requested review from hrdinka, kevincox and peti May 8, 2021 00:44
@lukegb
Copy link
Contributor

lukegb commented May 8, 2021

@ofborg build backblaze-b2

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels May 8, 2021
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. Why is this arrow/2.nix rather than e.g. arrow/0.nix...

@FRidh who added this in #117452 (see #117227)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm not sure, happy to rename it, nothing else is using it anyway.

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 it was from python2, not 2.0

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'll rename it then, backblaze is using it from python 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

that being said, this is a form of pinning, and b2sdk should really just adopt the newer version of arrow

Pinning within python-modules is highly discouraged. This will potentially introduce incompatible versions in a user's environment as either the new pinned version or the original version will be imported at runtime, breaking one of the packages.

The preference to handling this is to relax version bounds in the "install_requires" field. (could be in setup.py, pyproject.toml, requirements or others). In most cases, packages are still compatible with small API changes which may warrant a major version bump. We use test suites to verify that the package still works correctly.

If the package is still incompatible with the latest major version, then the most proper way to handle this is make an issue with the upstream package to adopt the latest major version. Or if upstream is not very responsive, you are free patch the source to make it compatible.

In very few circumstances, two versions of the same package are allowed to exist if the packages are extremely difficult to package. Some examples of this are tensorflow, which has huge ecosystems built around it and is hard to package. Another is django, which has 2 actively developed versions, and large ecosystems built around each.

One exception to this is applications, due to the way buildPythonApplication and toPythonApplication functions work, the related derivations will not bleed dependencies between packages. If the package doesn't need to be imported by other python modules, then this package would be a good candidate to convert into application. You can look at https://github.com/NixOS/nixpkgs/blob/master/pkgs/tools/admin/awscli/default.nix as an example of using an overlay within a python application.

Info on buildPythonApplication can be found here.
Info on toPythonApplication can be found here.

@siraben siraben self-requested a review May 8, 2021 02:09
@siraben
Copy link
Member

siraben commented May 8, 2021

Result of nixpkgs-review pr 122138 run on x86_64-darwin 1

9 packages built:
  • backblaze-b2
  • duplicity
  • duply
  • python38Packages.arrow_0
  • python38Packages.b2sdk
  • python38Packages.rst2ansi
  • python39Packages.arrow_0
  • python39Packages.b2sdk
  • python39Packages.rst2ansi

Copy link
Member

@siraben siraben left a comment

Choose a reason for hiding this comment

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

LGTM

@r-rmcgibbo
Copy link

r-rmcgibbo commented May 8, 2021

Result of nixpkgs-review pr 122138 at 9c0a6126 run on aarch64-linux 1

1 package failed to build:
9 packages built successfully:
  • deja-dup
  • duplicity
  • duply
  • python38Packages.arrow_0
  • python38Packages.b2sdk
  • python38Packages.rst2ansi
  • python39Packages.arrow_0
  • python39Packages.b2sdk
  • python39Packages.rst2ansi
4 suggestions:
  • warning: maintainers-missing

    Package does not have a maintainer. Consider adding yourself?

    Near pkgs/development/python-modules/b2sdk/default.nix:36:3:

       |
    36 |   meta = with lib; {
       |   ^
    
  • warning: no-python-tests

    Test runner could not discover any test cases: ‘Ran 0 tests in 0.000s’
    Near pkgs/development/python-modules/rst2ansi/default.nix:23:0:

       |
    23 |     description = "A rst converter to ansi-decorated console output";
       | ^
    
  • warning: unused-argument

    Unused argument: pytestCheckHook.
    Near pkgs/development/python-modules/b2sdk/default.nix:1:63:

      |
    1 | { lib, buildPythonPackage, fetchPypi, setuptools-scm, isPy27, pytestCheckHook
      |                                                               ^
    
  • warning: maintainers-missing

    Package does not have a maintainer. Consider adding yourself?

    Near pkgs/development/python-modules/rst2ansi/default.nix:20:3:

       |
    20 |   meta = with lib; {
       |   ^
    

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.


Result of nixpkgs-review pr 122138 at db648766 run on x86_64-linux 1

1 package failed to build:
9 packages built successfully:
  • deja-dup
  • duplicity
  • duply
  • python38Packages.arrow_0
  • python38Packages.b2sdk
  • python38Packages.rst2ansi
  • python39Packages.arrow_0
  • python39Packages.b2sdk
  • python39Packages.rst2ansi
3 suggestions:
  • warning: unused-argument

    Unused argument: chai.
    Near pkgs/development/python-modules/arrow/0.nix:2:9:

      |
    2 | , nose, chai, simplejson, backports_functools_lru_cache
      |         ^
    
  • warning: python-include-tests

    Consider adding a checkPhase for tests, or if not feasible, pythonImportsCheck.

    Near pkgs/development/python-modules/rst2ansi/default.nix:24:0:

       |
    24 |     description = "A rst converter to ansi-decorated console output";
       | ^
    
  • warning: unused-argument

    Unused argument: nose.
    Near pkgs/development/python-modules/arrow/0.nix:2:3:

      |
    2 | , nose, chai, simplejson, backports_functools_lru_cache
      |   ^
    

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

Comment on lines 18 to 20
Copy link
Contributor

Choose a reason for hiding this comment

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

default behavior

Suggested change
doCheck = true;

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add yourself as a maintainer when adding a new package.

https://nixos.org/manual/nixpkgs/stable/#reviewing-contributions-new-packages

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 saw a lot of other python packages didn't have maintainers, so I didn't realise this was the same as top level packages.

Comment on lines 23 to 25
Copy link
Contributor

Choose a reason for hiding this comment

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

looks a little cleaner, IMO

Suggested change
preBuild = ''
export SETUPTOOLS_SCM_PRETEND_VERSION="${version}"
'';
SETUPTOOLS_SCM_PRETEND_VERSION= version;

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 it was from python2, not 2.0

@siraben
Copy link
Member

siraben commented May 8, 2021

@uri-canva please fix the evaluation error on darwin

@siraben
Copy link
Member

siraben commented May 8, 2021

Result of nixpkgs-review pr 122138 run on x86_64-darwin 1

9 packages built:
  • backblaze-b2
  • duplicity
  • duply
  • python38Packages.arrow_0
  • python38Packages.b2sdk
  • python38Packages.rst2ansi
  • python39Packages.arrow_0
  • python39Packages.b2sdk
  • python39Packages.rst2ansi

@siraben siraben self-requested a review May 8, 2021 05:19
Copy link
Member

@siraben siraben left a comment

Choose a reason for hiding this comment

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

Looks good again

@fabaff
Copy link
Member

fabaff commented May 8, 2021

#121250 was already covers backblaze-b2 and b2sdk.

@uri-canva
Copy link
Contributor Author

Nice! I should I searched before starting on this. I have tested it on darwin and it's broken, but I'll wait for it to picked up by hydra to double check that it's not something wrong with my local setup: https://hydra.nixos.org/job/nixpkgs/gnome/backblaze-b2.x86_64-darwin/all.

@kevincox
Copy link
Contributor

kevincox commented May 8, 2021

Oh no. If it is broken on darwin let me know and I'll revert it. Was this change good on darwin? I didn't notice any obvious differences other than the arrow patch.

@uri-canva
Copy link
Contributor Author

This change is good on darwin, but the build was broken on darwin in a different way before your PR, I'm not sure if your PR layered a new break on top of it, or switched it, I will check now.

@uri-canva
Copy link
Contributor Author

Ok, tested again, it all works on darwin, so false alarm. Thanks @fabaff @volta001.

@uri-canva uri-canva closed this May 9, 2021
@uri-canva uri-canva deleted the b2sdk branch May 9, 2021 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants