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

Jump to bottom

matrix-synapse: prepare for python3 switch #49858

Merged
merged 1 commit into from Nov 9, 2018
Merged

matrix-synapse: prepare for python3 switch #49858

merged 1 commit into from Nov 9, 2018

Conversation

peterhoeg
Copy link
Member

Motivation for this change

Performance on a low-specced machine is not great, so enable move to py3 which upstream recommends.

I have not benchmarked it.

GH was wonky, so I had to create a new PR: #48695

Apologies for the noise.

Cc: @dotlambda @Ekleog @Ralith

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@peterhoeg peterhoeg requested a review from FRidh as a code owner November 7, 2018 06:17
@Ekleog
Copy link
Member

Ekleog commented Nov 7, 2018

@GrahamcOfBorg test matrix-synapse

@Ralith
Copy link
Contributor

Ralith commented Nov 7, 2018

Since affinity is no longer a dependency of synapse, I think it should just be removed outright; it has not been updated for more than a decade and does not appear to be referenced by any other expression.

@peterhoeg
Copy link
Member Author

Removing affinity should be done in a separate PR IMHO so I just dropped the commit from here.

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: matrix-synapse

Partial log (click to expand)

/nix/store/sxcwx27vj9dfw5b795bwv20w5marbvf5-matrix-synapse-0.33.8

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: matrix-synapse

Partial log (click to expand)

wrapping `/nix/store/b7p5j0appl83kp0cpkr9r7rc4da77liy-matrix-synapse-0.33.8/bin/move_remote_media_to_new_store.py'...
wrapping `/nix/store/b7p5j0appl83kp0cpkr9r7rc4da77liy-matrix-synapse-0.33.8/bin/register_new_matrix_user'...
wrapping `/nix/store/b7p5j0appl83kp0cpkr9r7rc4da77liy-matrix-synapse-0.33.8/bin/hash_password'...
wrapping `/nix/store/b7p5j0appl83kp0cpkr9r7rc4da77liy-matrix-synapse-0.33.8/bin/synapse_port_db'...
running install tests

-------------------------------------------------------------------------------

PASSED
pytestcachePhase

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: tests.matrix-synapse

Partial log (click to expand)

serversqlite: exit status 0
serverpostgres: running command: sync
serverpostgres: exit status 0
test script finished in 409.63s
cleaning up
killing serversqlite (pid 631)
killing serverpostgres (pid 644)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/9k2k916kfd17n3nfn9g54803cqvn0523-vm-test-run-matrix-synapse

dotlambda
dotlambda reviewed Nov 7, 2018
View reviewed changes
pkgs/servers/matrix-synapse/default.nix Outdated
buildInputs = with python2Packages; [
mock setuptoolsTrial
];
buildInputs = [ mock setuptoolsTrial ];
Copy link
Member

Choose a reason for hiding this comment

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

checkInputs?

pkgs/top-level/all-packages.nix Outdated
@@ -3720,7 +3720,9 @@ with pkgs;

makebootfat = callPackage ../tools/misc/makebootfat { };

matrix-synapse = callPackage ../servers/matrix-synapse { };
matrix-synapse = callPackage ../servers/matrix-synapse {
python = python2;
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not having this here but instead use python2 as an argument.
However, I think that comes down to personal preference.

Copy link
Member

Choose a reason for hiding this comment

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

Well, if the point is to allow switching to python3 and it's almost there, doesn't it make sense to take python? Until the switch can be complete and we can take python3 as an argument :)

Copy link
Member

Choose a reason for hiding this comment

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

Alright, why not.

Copy link
Member

Choose a reason for hiding this comment

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

One could have inside the expression:

let
  python = python2;
in ...

The idea is to keep all-packages.nix as clean as possible.

@peterhoeg
matrix-synapse: prepare for python3 switch
dc74c44 
With this commit, we *can* swap python2 for python3 to run synapse using python3
instead.

The reason for not making the switch is that a number of CLI tools provided with
synapse do not yet work under py3 despite synapse running fine.

So this doesn't actually do anything on its own except to prepare for the
upcoming py3 switch.
@peterhoeg
Copy link
Member Author

I hope this iteration is more to everyone's liking.

Please note that from 0.34.0, py3 is the recommended python ref matrix-org/synapse#4036

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: matrix-synapse

Partial log (click to expand)

wrapping `/nix/store/dkk30r5hjpx63w48ba4g2bxk5mfpif11-matrix-synapse-0.33.8/bin/synctl'...
wrapping `/nix/store/dkk30r5hjpx63w48ba4g2bxk5mfpif11-matrix-synapse-0.33.8/bin/move_remote_media_to_new_store.py'...
wrapping `/nix/store/dkk30r5hjpx63w48ba4g2bxk5mfpif11-matrix-synapse-0.33.8/bin/hash_password'...
wrapping `/nix/store/dkk30r5hjpx63w48ba4g2bxk5mfpif11-matrix-synapse-0.33.8/bin/homeserver'...
running install tests

-------------------------------------------------------------------------------

PASSED
pytestcachePhase

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: matrix-synapse

Partial log (click to expand)

wrapping `/nix/store/lc8cwzfl104z81a7v2qc41alx59ai2w9-matrix-synapse-0.33.8/bin/move_remote_media_to_new_store.py'...
wrapping `/nix/store/lc8cwzfl104z81a7v2qc41alx59ai2w9-matrix-synapse-0.33.8/bin/register_new_matrix_user'...
wrapping `/nix/store/lc8cwzfl104z81a7v2qc41alx59ai2w9-matrix-synapse-0.33.8/bin/synapse_port_db'...
wrapping `/nix/store/lc8cwzfl104z81a7v2qc41alx59ai2w9-matrix-synapse-0.33.8/bin/synctl'...
running install tests

-------------------------------------------------------------------------------

PASSED
pytestcachePhase

dotlambda
dotlambda approved these changes Nov 8, 2018
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.

LGTM

@peterhoeg peterhoeg merged commit 43922fe into NixOS:master Nov 9, 2018
@peterhoeg peterhoeg deleted the f/synapse branch November 9, 2018 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ekleog Ekleog Ekleog approved these changes

@dotlambda dotlambda dotlambda approved these changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@peterhoeg @Ekleog @Ralith @GrahamcOfBorg @FRidh @dotlambda