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

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 SFrijters commented May 23, 2020 •
edited

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.

@SFrijters SFrijters mentioned this pull request May 23, 2020
Closed
@ofborg ofborg bot requested a review from peterhoeg May 23, 2020 17:30
@peterhoeg
Copy link
Member

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
Copy link
Member Author

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

@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

If this version works, LGTM!

@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 •
edited

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 June 16, 2020 17:54
jonringer
jonringer reviewed Jun 16, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

sorry for the nit picks

pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/maestral/default.nix Show resolved Hide resolved
pkgs/development/python-modules/maestral/default.nix Outdated Show resolved Hide resolved
jonringer
jonringer reviewed Jun 16, 2020
View reviewed changes
pkgs/development/python-modules/maestral/default.nix Outdated
, systemd
}:

python.pkgs.buildPythonApplication rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 {

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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.

jonringer
jonringer approved these changes Jun 22, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

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
@SFrijters SFrijters deleted the maestral-update branch September 30, 2020 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@peterhoeg peterhoeg Awaiting requested review from peterhoeg

Assignees
No one assigned
Labels
6.topic: python 8.has: clean-up 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SFrijters @peterhoeg @jonringer