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

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)

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
@Valodim Valodim force-pushed the Valodim:etesync-dav/init branch from c862dc7 to 48a6bdd Jan 4, 2020
@gruns
Copy link

@gruns gruns commented Jan 4, 2020

@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

@jonringer jonringer commented Jan 4, 2020

@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 Valodim commented Jan 6, 2020

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 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 :)

@Valodim Valodim force-pushed the Valodim:etesync-dav/init branch from 3c00766 to fc4ecec Jan 22, 2020
@matt-snider
Copy link
Contributor

@matt-snider matt-snider commented Feb 18, 2020

@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

@jonringer jonringer commented Feb 18, 2020

also,o please resolve conflicts

@tasn
Copy link

@tasn 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.

@Valodim Valodim force-pushed the Valodim:etesync-dav/init branch 2 times, most recently from aa40591 to 04f8e48 Feb 19, 2020
@Valodim
Copy link
Contributor Author

@Valodim Valodim commented Feb 19, 2020

done and done.

Copy link
Contributor

@jonringer jonringer left a comment

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 Valodim force-pushed the Valodim:etesync-dav/init branch from 04f8e48 to 97e1f0f Feb 20, 2020
@Valodim
Copy link
Contributor Author

@Valodim Valodim commented Feb 20, 2020

squashed that in 👍

Copy link
Contributor

@jonringer jonringer left a comment

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

@jonringer jonringer commented Feb 21, 2020

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

@Valodim Valodim force-pushed the Valodim:etesync-dav/init branch from 97e1f0f to 1c0b00e Feb 21, 2020
@Valodim
Copy link
Contributor Author

@Valodim Valodim commented Feb 21, 2020

Done. Fixed version numbers, too.

@jonringer
Copy link
Contributor

@jonringer jonringer commented Feb 22, 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.

Copy link
Contributor

@jonringer jonringer left a comment

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

@jonringer jonringer commented Feb 22, 2020

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

@jonringer
Copy link
Contributor

@jonringer jonringer commented Feb 22, 2020

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

@jonringer jonringer merged commit bd91cac into NixOS:master Feb 22, 2020
18 checks passed
18 checks passed
etesync-dav, python27Packages.furl, python27Packages.pyscrypt, python37Packages.etesync, python37Packages.furl, python37Packages.pyscrypt, python38Packages.etesync, python38Packages.furl, python38Packages.pyscrypt on x86_64-darwin Failure
Details
Evaluation Performance Report Evaluator Performance Report
Details
etesync-dav on aarch64-linux Success
Details
etesync-dav on x86_64-linux Success
Details
etesync-dav, python27Packages.furl, python27Packages.pyscrypt, python37Packages.etesync, python37Packages.furl, python37Packages.pyscrypt, python38Packages.etesync, python38Packages.furl, python38Packages.pyscrypt on aarch64-linux Success
Details
etesync-dav, python27Packages.furl, python27Packages.pyscrypt, python37Packages.etesync, python37Packages.furl, python37Packages.pyscrypt, python38Packages.etesync, python38Packages.furl, python38Packages.pyscrypt on x86_64-linux Success
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="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
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
@tasn
Copy link

@tasn 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 Valodim:etesync-dav/init branch Feb 27, 2020
@matt-snider matt-snider mentioned this pull request Mar 27, 2020
5 of 10 tasks complete
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

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