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

maestral: 0.6.4 -> 1.1.0 #88698

Merged
merged 2 commits into from Jun 22, 2020
Merged

maestral: 0.6.4 -> 1.1.0 #88698

merged 2 commits into from Jun 22, 2020

Conversation

@SFrijters
Copy link
Member

SFrijters commented May 23, 2020

Motivation for this change

Maestral has seen a new major version released. In this release the GUI is split off into different packages (the GUI didn't work for me at all in the current nixpkgs version if I enabled the withGUI flag). So, I've moved the maestral base package to pythonPackages since it's imported as a library by the GUI versions. The cli-maestral is still available via toPythonApplication.

Since the program spawns a daemon it seems to need additions to the PYTHONPATH to work (but there is probably a better way I don't know about).

I don't have access to a Mac, so the new maestral-cocoa package is not packaged. It should be trivial to include in the same way. The package set attribute is the neutral maestral-gui to hide maestal-qt and maestral-cocoa.

This PR will also fix #87315.

I'm using this branch in my system configuration and it Works For Me (TM).

The commits will be cleaned up before I remove the WIP part.

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.
@peterhoeg
Copy link
Member

peterhoeg commented May 26, 2020

I think there's a better way to handle the wrapperargs as well not having to redo them when pythonPackages.maestral is a propagatedBuildInput but I need to check it out. Will only be later this week.

@SFrijters SFrijters force-pushed the SFrijters:maestral-update branch from 91466ee to d96a612 Jun 8, 2020
@SFrijters
Copy link
Member Author

SFrijters commented Jun 8, 2020

Rebased on master to stay up-to-date.

@SFrijters SFrijters changed the title WIP maestral: 0.6.4 -> 1.0.3 WIP maestral: 0.6.4 -> 1.1.0 Jun 15, 2020
@SFrijters
Copy link
Member Author

SFrijters commented Jun 15, 2020

@peterhoeg Any idea on how to improve this? I've just bumped the version to the latest (1.1.0), will test locally to see if it didn't break anything.

@peterhoeg
Copy link
Member

peterhoeg commented Jun 16, 2020

If this version works, LGTM!

@SFrijters SFrijters force-pushed the SFrijters:maestral-update branch from 5771175 to b0146db Jun 16, 2020
@SFrijters SFrijters changed the title WIP maestral: 0.6.4 -> 1.1.0 maestral: 0.6.4 -> 1.1.0 Jun 16, 2020
@SFrijters
Copy link
Member Author

SFrijters commented Jun 16, 2020

Rebased into two commits, one for the required dropbox update, and one for maestral itself.
EDIT: No actual code changes compared to the reviewed version.

@ofborg ofborg bot requested a review from peterhoeg Jun 16, 2020
Copy link
Contributor

jonringer left a comment

sorry for the nit picks

pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/maestral/default.nix Outdated Show resolved Hide resolved
@SFrijters SFrijters force-pushed the SFrijters:maestral-update branch from b0146db to 1841521 Jun 16, 2020
, systemd
}:

python.pkgs.buildPythonApplication rec {

This comment has been minimized.

Copy link
@jonringer

jonringer Jun 16, 2020

Contributor

sorry, i looked over this earlier, packages in python-modules need to use buildPythonPackage, and generally python packages are imported individually

Suggested change
python.pkgs.buildPythonApplication rec {
buildPythonPackage rec {

This comment has been minimized.

Copy link
@SFrijters

SFrijters Jun 17, 2020

Author Member

Done. I've also explicitly imported pythonOlder since that seems to follow that style as well.

This comment has been minimized.

Copy link
@jonringer

jonringer Jun 17, 2020

Contributor

also meant to link this video on python packaging for nix https://www.youtube.com/watch?v=jXd-hkP4xnU&t=3s

I think i mention briefly about the different between Package and Application

This comment has been minimized.

Copy link
@SFrijters

SFrijters Jun 17, 2020

Author Member

It was a nice vid to watch, but it didn't cover that issue in particular. I'll keep these things in mind for the future though. The reason this package ended up in the state it was in is mostly because in the 0.6.4 version the maestral package was just a standalone application, so I ended up moving the nix code into python-modules wholesale. And it just worked so I didn't think to look at the other python modules to compare, which was my bad.

Please let me know if I can squash my new commit or if there are any other niggles.

@SFrijters SFrijters force-pushed the SFrijters:maestral-update branch from daa38db to 776597b Jun 17, 2020
@SFrijters SFrijters force-pushed the SFrijters:maestral-update branch from 776597b to 008a335 Jun 21, 2020
@SFrijters
Copy link
Member Author

SFrijters commented Jun 21, 2020

There has been a bulk update of Python packages. I've removed my dropbox update because it's already been updated on master, but I've had to add a new commit for the updated Pyro package, which fails some networking tests that have been added between 5.7 and 5.10.

Copy link
Contributor

jonringer left a comment

LGTM
gui opens fine

https://github.com/NixOS/nixpkgs/pull/88698
5 packages built:
maestral maestral-gui python37Packages.Pyro5 python37Packages.maestral python38Packages.Pyro5
@jonringer jonringer merged commit 4310704 into NixOS:master Jun 22, 2020
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d028a47"; rev="d028a474cacec950cb492aaac0375d9d84d26866"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d028a47"; rev="d028a474cacec950cb492aaac0375d9d84d26866"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d028a47"; rev="d028a474cacec950cb492aaac0375d9d84d26866"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d028a47"; rev="d028a474cacec950cb492aaac0375d9d84d26866"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d028a47"; rev="d028a474cacec950cb492aaac0375d9d84d26866"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d028a47"; rev="d028a474cacec950cb492aaac0375d9d84d26866"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d028a47"; rev="d028a474cacec950cb492aaac0375d9d84d26866"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
maestral, maestral.passthru.tests on aarch64-linux Success
Details
maestral, maestral.passthru.tests on x86_64-linux Success
Details
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

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