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

python: pytest: 5.2.4 -> 5.3.1 #74917

Closed
wants to merge 1 commit into from
Closed

Conversation

@davidak
Copy link
Member

@davidak davidak commented Dec 3, 2019

Motivation for this change

New release

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 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.
Notify maintainers

cc @FRidh

@jonringer
Copy link
Contributor

@jonringer jonringer commented Dec 3, 2019

generally, minor bumps in pytest cause breaks in many package. See #72825 for more context

@davidak
Copy link
Member Author

@davidak davidak commented Dec 3, 2019

@jonringer so we need more tests to test it automatically? :)

@jonringer
Copy link
Contributor

@jonringer jonringer commented Dec 4, 2019

a lot of the python packages have tests, it's just that this will kick off around 5k-8k rebuilds per platform, some of which may break

@FRidh
Copy link
Member

@FRidh FRidh commented Dec 4, 2019

Tests itself are one thing. The hard part is decision making based on the results of the tests; it never happens that everything passes so how do you choose whether to go ahead or not. See e.g. how we use channels in Nixpkgs.

@davidak
Copy link
Member Author

@davidak davidak commented Dec 4, 2019

how do you choose whether to go ahead or not.

So i touched again a topic that is missing a clear policy? I hoped this is just a simple update.

I think it's not an option to not update packages. We should automate the process as much as possible and make updating simple. And hopefully automate updating completely in the future. That is my vision.

In this case i would try to fix all breaking packages by updating them too.

I tried to build and test the depending packages with nix-review but the tool seems broken.

[davidak@ethmoid:~/code/nixpkgs]$ nix-shell -p nix-review --run "nix-review pr 74917"
$ git fetch --force https://github.com/NixOS/nixpkgs master:refs/nix-review/0 pull/74917/head:refs/nix-review/1
From https://github.com/NixOS/nixpkgs
 * [new ref]                 refs/pull/74917/head -> refs/nix-review/1
$ git worktree add /home/davidak/.cache/nix-review/pr-74917/nixpkgs b6284fd70f7b4435b58fe7f97532454aa23de327
Preparing worktree (detached HEAD b6284fd70f7)
Updating files: 100% (20399/20399), done.
HEAD is now at b6284fd70f7 libreoffice: use external Poppler 0.83 patch
$ git merge --no-commit 34b7bb566c712338b99edc780f4270d94c9f6c1b
Automatic merge went well; stopped before committing as requested
$ nix build --no-link --keep-going --max-jobs 4 --option build-use-sandbox true -f /home/davidak/.cache/nix-review/pr-74917/build.nix
error: syntax error, unexpected IF, expecting ID or OR_KW or DOLLAR_CURLY or '"', at /home/davidak/.cache/nix-review/pr-74917/build.nix:3992:21

https://github.com/NixOS/nixpkgs/pull/74917
197 package are marked as broken and were skipped:
AgdaSheaves TotalParserCombinators agdaBase aldor antimicro
...
12435 package failed to build:
AgdaStdlib DisnixWebService EmptyEpsilon R SDL SDL2 SDL2_gfx
...
error: build log of '/nix/store/nv7s4dkmk9gr8sp5d9qbp91k1z4bz33y-dumb-2.0.3.drv' is not available
[0.0 MiB DL]
error: build log of '/nix/store/p2x22d5n39p6p5mp6jizizk0z5xzj3kj-dvc-0.24.3.drv' is not available
[0.0 MiB DL]
error: build log of '/nix/store/1v81b5rdg5jqlb1kw4ykldg57jnimib7-dvc-0.24.3.drv' is not available
[0.0 MiB DL]
error: build log of '/nix/store/d9h975vks4slzirvm5h9zy2g0q1688kq-dvd-slideshow-0.8.4-2.drv' is not available
...

How would you go further with pytest? Close this PR and wait for an important update? Wait for some time like 3 months?

@FRidh
Copy link
Member

@FRidh FRidh commented Dec 4, 2019

Could you seriously stop the complaining? Thinking one can "just" automatically update everything is extremely shortsighted. It would require setting up rules and thresholds for all packages along with extensive CI that is just not going to happen. This is a mass-rebuild, and with mass-rebuilds we need to be careful. This is a package that is known to cause breakage upon updates. Therefore, it warrants extra care. How that is done is in the end up to the package maintainers.

Not updating is an option, and is in fact often the sane one as well. Continuously updating is expensive because of the high amount of work it takes, and it is thus doubtful whether there is any net gain in doing so.

@lsix
Copy link
Member

@lsix lsix commented Dec 4, 2019

Given the amount of breakage that pytest tends to produce, I tend to only consider updating it when it is required for something depending on the update.

This is far from perfect, but pytest updates are a pain otherwise.

So my personal guideline is: if someone needs an update, lets go for it, if it is an update just for an update, it might not be worth the effort (pytest is a build-time dependency, no impact on production systems, I would not have the same approach for software that actually run on production).

@davidak
Copy link
Member Author

@davidak davidak commented Dec 4, 2019

Sorry. I leave updating pytest to the maintainers then.

@davidak davidak closed this Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.