Skip to content

python3Packages.sanic: 21.3.2 -> 21.3.4; fix tests#120881

Merged
dotlambda merged 4 commits intoNixOS:masterfrom
AluisioASG:aasg/sanic-routing
Apr 30, 2021
Merged

python3Packages.sanic: 21.3.2 -> 21.3.4; fix tests#120881
dotlambda merged 4 commits intoNixOS:masterfrom
AluisioASG:aasg/sanic-routing

Conversation

@AluisioASG
Copy link
Contributor

@AluisioASG AluisioASG commented Apr 27, 2021

Sanic 21.3.0 split up parts of itself into new packages that are consumed as dependencies, while those packages themselves need Sanic for testing, so we need a few doCheck = false here and there to break loops.

pytest-sanic is in the odd situation that it doesn't work with Sanic 21.3 (yunstanford/pytest-sanic#50) yet it is needed to run Sanic's tests. Right now I'm just disabling checks for this combination of versions, but it might be better to mark pytest-sanic as broken and override that in sanic instead.

cc @costrouc

Motivation for this change

@dotlambda discovered that python3Packages.sanic was broken in #120447 (comment).

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.

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Apr 27, 2021
@AluisioASG AluisioASG requested a review from costrouc April 27, 2021 15:46
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Apr 27, 2021
@r-rmcgibbo
Copy link

r-rmcgibbo commented Apr 27, 2021 •
edited
Loading

Result of nixpkgs-review pr 120881 at 5923f77a run on aarch64-linux 1

2 packages failed to build:
12 packages built successfully:
  • python38Packages.entrance
  • python38Packages.entrance-with-router-features
  • python38Packages.pytest-sanic
  • python38Packages.sanic
  • python38Packages.sanic-routing
  • python38Packages.sanic-testing
  • python39Packages.entrance
  • python39Packages.entrance-with-router-features
  • python39Packages.pytest-sanic
  • python39Packages.sanic
  • python39Packages.sanic-routing
  • python39Packages.sanic-testing
2 suggestions:
  • warning: duplicate-check-inputs

    Dependencies listed in propagatedBuildInputs should not be repeated in checkInputs.

    Detected duplicates:

    • python3.8-multidict-5.1.0
    • python3.8-uvloop-0.15.2
    • python3.8-ujson-4.0.2

    Near pkgs/development/python-modules/sanic/default.nix:32:3:

       |
32 |   checkInputs = [
   |   ^

    Near pkgs/development/python-modules/sanic/default.nix:28:3:

       |
28 |   propagatedBuildInputs = [
   |   ^
  • warning: duplicate-check-inputs

    Dependencies listed in propagatedBuildInputs should not be repeated in checkInputs.

    Detected duplicates:

    • python3.9-multidict-5.1.0
    • python3.9-uvloop-0.15.2
    • python3.9-ujson-4.0.2

    Near pkgs/development/python-modules/sanic/default.nix:32:3:

       |
32 |   checkInputs = [
   |   ^

    Near pkgs/development/python-modules/sanic/default.nix:28:3:

       |
28 |   propagatedBuildInputs = [
   |   ^

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement.

Result of nixpkgs-review pr 120881 at 049d419a run on x86_64-linux 1

6 packages failed to build:
8 packages built successfully:
  • python38Packages.entrance
  • python38Packages.entrance-with-router-features
  • python38Packages.pytest-sanic
  • python38Packages.sanic
  • python38Packages.sanic-auth
  • python38Packages.sanic-routing
  • python38Packages.sanic-testing
  • python39Packages.pytest-sanic
2 suggestions:
  • warning: duplicate-check-inputs

    Dependencies listed in propagatedBuildInputs should not be repeated in checkInputs.

    Detected duplicates:

    • python3.9-sanic-testing-0.3.1

    Near pkgs/development/python-modules/sanic/default.nix:33:3:

       |
33 |   checkInputs = [
   |   ^

    Near pkgs/development/python-modules/sanic/default.nix:28:3:

       |
28 |   propagatedBuildInputs = [
   |   ^
  • warning: duplicate-check-inputs

    Dependencies listed in propagatedBuildInputs should not be repeated in checkInputs.

    Detected duplicates:

    • python3.8-sanic-testing-0.3.1

    Near pkgs/development/python-modules/sanic/default.nix:33:3:

       |
33 |   checkInputs = [
   |   ^

    Near pkgs/development/python-modules/sanic/default.nix:28:3:

       |
28 |   propagatedBuildInputs = [
   |   ^

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement.

@AluisioASG
Copy link
Contributor Author

AluisioASG commented Apr 27, 2021

sanic-testing needed to be moved to propagatedBuildInputs. On the bright side, we can disable tests only when the new packages are used as dependencies of sanic.

dotlambda
dotlambda requested changes Apr 29, 2021
View reviewed changes
pkgs/top-level/python-packages.nix Outdated
Comment on lines 7188 to 7207
Copy link
Member

@dotlambda dotlambda Apr 29, 2021

Choose a reason for hiding this comment

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

This is the wrong way round: We should only override packages that are not in propagatedBuildInputs.

Copy link
Contributor Author

@AluisioASG AluisioASG Apr 29, 2021

Choose a reason for hiding this comment

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

Any suggestions? Off the top of my head, tests have to be disabled either here or at the top level, or else we have to build everything twice (first with tests off, then use those to build with tests on).

Copy link
Member

@dotlambda dotlambda Apr 29, 2021

Choose a reason for hiding this comment

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

sanic-routing and sanic-testing have sanic in their checkInputs, so we can override their input sanic.

Copy link
Contributor Author

@AluisioASG AluisioASG Apr 29, 2021

Choose a reason for hiding this comment

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

If I got this right, it'd the last option I proposed: 1) build sanic-routing and sanic-testing without tests; 2) use those as input to an intermediate sanic; 3) use it to build sanic-routing and sanic-testing with tests; and 4) use those as inputs to the final sanic.

And the lines you highlighted go into the overrides for sanic-routing and sanic-testing, leaving the top-level sanic with no overrides.

Copy link
Member

@dotlambda dotlambda Apr 29, 2021

Choose a reason for hiding this comment

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

Oh, true. I forgot that sanic doesn't build without those dependencies. If there's no way to do that we should just turn off sanic-routing's and sanic-testing's tests.

Copy link
Contributor Author

@AluisioASG AluisioASG Apr 30, 2021

Choose a reason for hiding this comment

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

So it turns out that sanic-routing doesn't actually depend on sanic for anything, despite it being listed in tests_require, so that package is no longer an issue.

As for sanic-testing, my reading of the situation is:

  • sanic only depends on sanic-testing for tests and some backwards compatibility (see also Remove test client sanic-org/sanic#2009);
  • sanic-testing does depend on sanic at runtime (the imports check can't even be done when it's not present).

Therefore I've added sanic as a propagated build input of sanic-testing, with special logic to disable all testing when the former is explicitly set to null (which we do when building sanic). Said special logic is only to avoid having to call both override and overridePythonAttrs in sequence in python-packages.nix.

All in all, this should give us three fully tested packages at the top level.

SuperSandro2000
SuperSandro2000 reviewed Apr 29, 2021
View reviewed changes
@AluisioASG
Copy link
Contributor Author

AluisioASG commented Apr 30, 2021

I went ahead and marked pytest-sanic as broken for the current version of sanic, while unbreaking it solely for sanic's tests.

dotlambda
dotlambda reviewed Apr 30, 2021
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Thanks for finding a good solution!

@AluisioASG
python3Packages.pytest-sanic: mark as broken with sanic >= 21.3.0
5b97159 
pytest-sanic is incompatible with the current version of Sanic, see
sanic-org/sanic#2095 and yunstanford/pytest-sanic#50.  While it is
broken, we also need it to run Sanic's tests (for which case it works
just fine).
@AluisioASG
python3Packages.sanic-routing: init at 0.6.2
550bb02
@AluisioASG
python3Packages.sanic-testing: init at 0.3.1
192b28a
@AluisioASG
python3Packages.sanic: 21.3.2 -> 21.3.4
bd815d2 
While we're at it, revise the dependencies lists; there's been a couple
of break-ups with 21.3.0.
@dotlambda dotlambda merged commit dc68eb5 into NixOS:master Apr 30, 2021
@dotlambda
Copy link
Member

dotlambda commented Apr 30, 2021 •
edited
Loading

If want to make another PR fixing sanic-auth, go ahead ;-)

@AluisioASG AluisioASG mentioned this pull request Apr 30, 2021
Merged
10 tasks
AluisioASG added a commit to AluisioASG/nixpkgs that referenced this pull request Apr 30, 2021
@AluisioASG
python3Packages.sanic-auth: fix tests
490b147 
After NixOS#120881, packages using Sanic's `app.test_client` or
`app.asgi_client` need to depend on `sanic-testing` as well.
@AluisioASG AluisioASG deleted the aasg/sanic-routing branch April 30, 2021 14:27
@AluisioASG AluisioASG mentioned this pull request Apr 30, 2021
Merged
10 tasks
dotlambda pushed a commit that referenced this pull request Apr 30, 2021
@AluisioASG
python3Packages.sanic-auth: fix tests (#121279)
0463f91 
After #120881, packages using Sanic's `app.test_client` or
`app.asgi_client` need to depend on `sanic-testing` as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dotlambda dotlambda dotlambda requested changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@costrouc costrouc Awaiting requested review from costrouc

Assignees

No one assigned

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: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AluisioASG @r-rmcgibbo @dotlambda @SuperSandro2000