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

vdirsyncerStable: init at 0.16.7 #59010

Merged
merged 2 commits into from Apr 8, 2019
Merged

Conversation

loewenheim
Copy link
Contributor

@loewenheim loewenheim commented Apr 5, 2019

Motivation for this change

Adds a package for the latest stable release of vdirsyncer.
fixes #58563

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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

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.

I'd prefer adding a stable.nix in the vdirsyncer directory.

pkgs/tools/misc/vdirsyncerStable/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/vdirsyncerStable/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/vdirsyncerStable/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/vdirsyncerStable/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/vdirsyncerStable/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/vdirsyncerStable/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/vdirsyncerStable/default.nix Outdated Show resolved Hide resolved

checkPhase = ''
rm -rf vdirsyncer
export PYTHONPATH=$out/${python3Packages.python.sitePackages}:$PYTHONPATH
Copy link
Member

Choose a reason for hiding this comment

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

This line is probably not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Why did you just remove the entire checkPhase? Please keep the third line.

pkgs/tools/misc/vdirsyncerStable/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/vdirsyncerStable/default.nix Outdated Show resolved Hide resolved
@loewenheim
Copy link
Contributor Author

Thank you for the review. I’ll make changes as soon as I’m able.

@dotlambda dotlambda changed the title vdirsyncerStable: Init at 0.16.7 vdirsyncerStable: init at 0.16.7 Apr 5, 2019
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.

Please squash your new commits into the existing one.

{ lib, python3Packages, fetchpatch }:

# Packaging documentation at:
# https://github.com/pimutils/vdirsyncer/blob/dcf5f701b7b5c21a8f4e8c80243db3e0baff1313/docs/packaging.rst
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# https://github.com/pimutils/vdirsyncer/blob/dcf5f701b7b5c21a8f4e8c80243db3e0baff1313/docs/packaging.rst
# https://github.com/pimutils/vdirsyncer/blob/0.16.7/docs/packaging.rst

description = "Synchronize calendars and contacts";
platforms = platforms.all;
license = licenses.mit;
};
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, should I just add myself to the maintainer list?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please do so in a separate commit preceding the existing one.

meta = with lib; {
homepage = https://github.com/pimutils/vdirsyncer;
description = "Synchronize calendars and contacts";
platforms = platforms.all;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
platforms = platforms.all;

That's set automatically by buildPythonApplication.

requests_oauthlib # required for google oauth sync
atomicwrites
milksnake
shippai
Copy link
Member

Choose a reason for hiding this comment

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

# for setuptools_scm:
echo 'Version: ${version}' >PKG-INFO

sed -i 's/spec.add_external_build(cmd=cmd/spec.add_external_build(cmd="true"/g' setup.py
Copy link
Member

Choose a reason for hiding this comment

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

Most of these lines (except for the first three) should be removed.

@dotlambda
Copy link
Member

@gebner @matthiasbeyer Many of my comments also apply to the vdirsyncer expression. It would be nice if you could improve it.

@gebner
Copy link
Member

gebner commented Apr 5, 2019

@dotlambda Thanks for looking over it, I've made the changes to the default vdirsyncer expression.

gebner added a commit that referenced this pull request Apr 5, 2019
homepage = https://github.com/pimutils/vdirsyncer;
description = "Synchronize calendars and contacts";
license = licenses.mit;
maintainers = [ loewenheim ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
maintainers = [ loewenheim ];
maintainers = with maintainers; [ loewenheim ];

The maintainers are in lib.maintainers. This error here causes evaluation to fail, as you can see from the failing @GrahamcOfBorg check below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Is it possible to run the check on my machine so I can see if it works?

Copy link
Member

Choose a reason for hiding this comment

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

env VERBOSE=1 ./maintainers/scripts/eval-release.sh

vdirsyncerStable: fixed maintainers
@loewenheim
Copy link
Contributor Author

Thank you again @dotlambda @gebner for your guidance.

@matthiasbeyer
Copy link
Contributor

Awesome! Now ... may I ask what "stable" means in the context of vdirsyncer? Because I really don't see how a "0.x.y" package can be "stable".

Or, phrased in another way: How do you want to update the new "stable" package? Only from 0.16.x -> 0.16.y?

@dotlambda
Copy link
Member

As you can see here, the 0.17.0 releases are still considered alpha. Hence I suggest keeping the vdirsyncerStable expression until these are considered stable, then making it an alias for vdirsyncer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vdirsyncer fails to sync
6 participants