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

package etesync-dav #76886

Merged
merged 4 commits into from Feb 22, 2020
Merged

package etesync-dav #76886

merged 4 commits into from Feb 22, 2020

Conversation

Valodim
Copy link
Contributor

@Valodim Valodim commented Jan 3, 2020

Motivation for this change

Packaged the etesync-dav component of etesync :)

This PR includes python library dependencies pyscrypt, orderedmultidict, and furl.

The furl package had a failing unit test, which I filed a bug report for: gruns/furl#121

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.
Notify maintainers

cc @tasn (etesync), @ricmoo (pyscrypt), @gruns (orderedmultidict, furl)

jonringer
jonringer reviewed Jan 3, 2020
View reviewed changes
maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/etesync-dav/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/furl/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/furl/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pyscrypt/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pyscrypt/default.nix Outdated Show resolved Hide resolved
@gruns
Copy link

gruns commented Jan 4, 2020 •
edited

@Valodim Weighing in here: is there anything in furl that blocks this PR, eg the test that mysteriously fails for you (gruns/furl#121)?

@jonringer
Copy link
Contributor

@gruns nix builds these packages in a sandboxed environment (chroot + no network + no FHS), so there's a large possibility that some intended test behavior may be invalidated. Doesn't mean it can't be worked around.

@Valodim
Copy link
Contributor Author

Valodim commented Jan 6, 2020 •
edited

the failing test doesn't look like it would be caused by sandboxing to me. it concerns edge case behavior of furl to mirror that of urllib. upstream should probably figure out what's going on, but from nixpkg's point of view I guess it's not terribly important to have it fixed before we package it.

looks like @gruns is on the case, anyways. thanks for looking into it :)

@Valodim
Copy link
Contributor Author

Valodim commented Jan 22, 2020

I think this is good to merge now. I've been using it for a couple days now, works as intended for me.

The one failing test in furl doesn't seem like a dealbreaker to me (it tests for consistency with urllib behavior in edge cases), and if upstream ever figures out what's going on we can enable it again :)

@matt-snider
Copy link
Contributor

matt-snider commented Feb 18, 2020 •
edited

@Valodim There's been some minor releases since you created the PR. For etesync/-dav it might be good to bump them and see if everything still works properly

@jonringer
Copy link
Contributor

also,o please resolve conflicts

@tasn
Copy link

tasn commented Feb 19, 2020

Hey, maintainer of etesync here.
I just noticed you are packaging "pyscrypt". That's fine, but just fyi, the package scrypt is faster (uses opensssl), so it's better to use that if possible. I'll take a look at changing the pypi package to depend on that by default and have pyscript as an alternative to avoid such confusing in the future.

Looking forward to seeing etesync-dav packaged.

@ofborg ofborg bot requested a review from vanzef February 19, 2020 09:42
@Valodim Valodim force-pushed the etesync-dav/init branch 2 times, most recently from aa40591 to 04f8e48 Compare February 19, 2020 12:45
@Valodim
Copy link
Contributor Author

Valodim commented Feb 19, 2020

done and done.

jonringer
jonringer reviewed Feb 20, 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.

to avoid:

==================================== ERRORS ====================================
__________________ ERROR collecting tests/test_collections.py __________________
/nix/store/7lx9rx7m8sc9wpjqkng5r4c3fy2mn3bv-python2.7-pytest-4.6.8/lib/python2.7/site-packages/_pytest/python.py:507: in _importtestmodule
    mod = self.fspath.pyimport(ensuresyspath=importmode)
/nix/store/905b3mx703j6an71ydz9s0lwi0c1a70w-python2.7-py-1.8.1/lib/python2.7/site-packages/py/_path/local.py:701: in pyimport
    __import__(modname)
E     File "/build/etesync-0.9.2/tests/test_collections.py", line 144
E   SyntaxError: Non-ASCII character '\xd7' in file /build/etesync-0.9.2/tests/test_collections.py on line 144, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!

I guess it technically is erroring on tests, and might be python2 compatible. But likely not

pkgs/development/python-modules/etesync/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/etesync/default.nix Outdated Show resolved Hide resolved
@Valodim
Copy link
Contributor Author

Valodim commented Feb 20, 2020

squashed that in 👍

jonringer
jonringer reviewed Feb 21, 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.

commit history should be:

pythonPackages.pyscrypt: init at 1.6.2
pythonPackages.furl: 2.0.0 -> 2.1.0
pythonPackages.etesync: init at 0.9.2
etesync-dav: init at 0.14.0

@jonringer
Copy link
Contributor

i would recommend using git rebase -i HEAD~4 and using the reword command

@Valodim
Copy link
Contributor Author

Valodim commented Feb 21, 2020

Done. Fixed version numbers, too.

@jonringer
Copy link
Contributor

Hey, maintainer of etesync here.
I just noticed you are packaging "pyscrypt". That's fine, but just fyi, the package scrypt is faster (uses opensssl), so it's better to use that if possible. I'll take a look at changing the pypi package to depend on that by default and have pyscript as an alternative to avoid such confusing in the future.

Looking forward to seeing etesync-dav packaged.

@tasn thanks for the heads up!

I'm not a user of this package, so I'll defer to @Valodim as to what should be in it. But we can always improve the packaging later. I'm mostly concerned about usability and correctness.

jonringer
jonringer approved these changes Feb 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.

diff LGTM
commits LGTM

[14 built, 11 copied (5.4 MiB), 1.3 MiB DL]
https://github.com/NixOS/nixpkgs/pull/76886
9 package built:
etesync-dav python27Packages.furl python27Packages.pyscrypt python37Packages.etesync python37Packages.furl python37Packages.pyscrypt python38Packages.etesync python38Packages.furl python38Packages.pyscrypt

@jonringer
Copy link
Contributor

@GrahamcOfBorg build etesync-dav python27Packages.furl python27Packages.pyscrypt python37Packages.etesync python37Packages.furl python37Packages.pyscrypt python38Packages.etesync python38Packages.furl python38Packages.pyscrypt

@jonringer
Copy link
Contributor

darwin failures don't seem to be related to this PR

@jonringer jonringer merged commit bd91cac into NixOS:master Feb 22, 2020
@tasn
Copy link

tasn commented Feb 24, 2020

Hey, maintainer of etesync here.
I just noticed you are packaging "pyscrypt". That's fine, but just fyi, the package scrypt is faster (uses opensssl), so it's better to use that if possible. I'll take a look at changing the pypi package to depend on that by default and have pyscript as an alternative to avoid such confusing in the future.
Looking forward to seeing etesync-dav packaged.

@tasn thanks for the heads up!

I'm not a user of this package, so I'll defer to @Valodim as to what should be in it. But we can always improve the packaging later. I'm mostly concerned about usability and correctness.

Fair enough, it doesn't really matter, but it's just something I noticed. Great to see it merged. :)

@Valodim Valodim deleted the etesync-dav/init branch February 27, 2020 16:52
@matt-snider matt-snider mentioned this pull request Mar 27, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matt-snider matt-snider matt-snider left review comments

@jonringer jonringer jonringer approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@vanzef vanzef Awaiting requested review from vanzef

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Valodim @gruns @jonringer @matt-snider @tasn @Janik-Haag