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: uvloop: fix build on all platforms #91488

Closed
wants to merge 1 commit into from

Conversation

simonchatts
Copy link
Contributor

uvloop builds currently fail on all platforms due to a file missing from the PyPi tarball. This temporarily patches the issue.

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.

@FRidh
Copy link
Member

FRidh commented Jun 25, 2020

typically we just disable flake8 testing because its of no use to us.

@simonchatts
Copy link
Contributor Author

Makes sense - I've re-tested the update on darwin and nixos. Thanks for the super-quick comments!

@jonringer
Copy link
Contributor

Upstream will be skipping some tests as well MagicStack/uvloop#318 (comment)

@simonchatts
Copy link
Contributor Author

Thanks @jonringer - looks like the upstream HEAD now has a fix for the test_sock_cancel_add_reader_race test, in response to your feedback. It would definitely be nice to drop the workaround in nixpkgs.

The issue here, though, is with a different failing test (which also has an unreleased upstream fix). So the obvious options seem to be:

  • drop both workarounds, pull in the latest dev commit, and trust that it's stable enough
  • add a temporary workaround for this second issue (the current PR contents)

The first option doesn't seem totally outrageous - the dev branch was named 0.15.0.dev0 in October, and subsequent commits look like stability fixes. But I have no idea what behaviour might have changed since the 0.14.0 release that other packages think they're depending on.

Which option would you prefer? I'm happy to update the PR if it's the first one (or a totally different solution).

@simonchatts
Copy link
Contributor Author

Any comment @FRidh or @jonringer about how you’d like to proceed with this? Currently this just disables flake8 testing, but happy to change if preferable.

@infinisil
Copy link
Member

Looks like this was fixed in #95591 now, thanks for the PR though :)

@infinisil infinisil closed this Aug 23, 2020
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

4 participants