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

vdirsyncer: deprecate vdirsyncerStable #103073

Merged
merged 3 commits into from
Nov 16, 2020
Merged

Conversation

magnetophon
Copy link
Member

@magnetophon magnetophon commented Nov 7, 2020

Motivation for this change
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.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Nov 7, 2020

khal:

looking for now-outdated files... none found
pickling environment... done
checking consistency... /build/khal-0.10.2/doc/source/man.rst: WARNING: document isn't included in any toctree
done
writing... khal.1 { usage configure standards faq license } /build/khal-0.10.2/doc/source/usage.rst:53: WARNING: unknown option: --color
/build/khal-0.10.2/doc/source/usage.rst:53: WARNING: unknown option: --color
/build/khal-0.10.2/doc/source/usage.rst:395: WARNING: unknown option: --interactive
/build/khal-0.10.2/doc/source/usage.rst:395: WARNING: unknown option: -i
/build/khal-0.10.2/doc/source/configure.rst:17: WARNING: unknown option: -c
path/to/config
done
build succeeded, 7 warnings.

The manual pages are in build/man.

make: /bin/sh: No such file or directory
make: *** [Makefile:133: man] Error 127
make: Leaving directory '/build/khal-0.10.2/doc'

@magnetophon
Copy link
Member Author

@SuperSandro2000 Thanks for the feedback, but I am not getting those errors.
When I run nix-env -f $NIXPKGS -iA khal, I do get build succeeded, 7 warnings., but I don't get those make errors.

Both vdirsyncer sync and khal list -o work as expected here.

How did you install khal?

@jonringer
Copy link
Contributor

@SuperSandro2000 is that on darwin?

Copy link
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

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

Looks good!

@SuperSandro2000
Copy link
Member

@SuperSandro2000 is that on darwin?

yes but now it fails with

tests/khalendar_test.py .............................                    [ 58%]                                                                                                   [20/48634]tests/khalendar_utils_test.py .......................................    [ 71%]                                                                                                             tests/parse_datetime_test.py ........................................... [ 86%]                                                                                                             ....                                                                     [ 87%]                                                                                                             tests/settings_test.py ............                                      [ 91%]                                                                                                             tests/terminal_test.py ...                                               [ 92%]                                                                                                             tests/utils_test.py .......                                              [ 95%]                                                                                                             tests/vdir_test.py X..                                                   [ 96%]                                                                                                             tests/vtimezone_test.py ...                                              [ 97%]                                                                                                             tests/ui/test_calendarwidget.py ...                                      [ 98%]                                                                                                             tests/ui/test_editor.py s...                                             [ 99%]                                                                                                             tests/ui/test_widgets.py .                                               [100%]                                                                                                                                                                                                                                                                                                         =================================== FAILURES ===================================                                                                                                            ___________________ test_vertical_month_unicode_weekdeays_gr ___________________                                                                                                                                                                                                                                                                                                            def test_vertical_month_unicode_weekdeays_gr():                                                                                                                                                 try:                                                                                                                                                                                            locale.setlocale(locale.LC_ALL, 'el_GR.UTF-8')                                                                                                                                              vert_str = vertical_month(month=12, year=2011,                                                                                                                                                                        today=dt.date(2011, 12, 12))
            # on some OSes, Greek locale's abbreviated day of the week and
            # month names have accents, on some they haven't
>           assert strip_accents('\n'.join([line.lower() for line in vert_str])) == \
                '\n'.join(example_gr)
E               AssertionError: assert '\x1b[1m    δ...  1  2  3  4 ' == '\x1b[1m     ...  1  2  3  4 '
E                 -      δε τρ τε πε πα σα κυ
E                 ?         -
E                 +     δε τρ τε πε πα σα κυ
E                 - δεκ  28 29 30  1  2  3  4
E                 ?         -
E                 + δεκ 28 29 30  1  2  3  4
E                 -       5  6  7  8  9 10 11 ...
E
E                 ...Full output truncated (39 lines hidden), use '-vv' to show

tests/cal_display_test.py:368: AssertionError
_________________________ test_vertical_month_abbr_fr __________________________

    def test_vertical_month_abbr_fr():
        # see issue #653
        try:
            locale.setlocale(locale.LC_ALL, 'fr_FR.UTF-8')
            vert_str = vertical_month(month=12, year=2011,
                                      today=dt.date(2011, 12, 12))
>           assert '\n'.join(vert_str) == '\n'.join(example_fr)
E           AssertionError: assert '\x1b[1m    L...  1  2  3  4 ' == '\x1b[1m     ...  1  2  3  4 '
E             -       lu ma me je ve sa di
E             ?         ^^^  ^  ^  ^  ^  ^  ^
E             +     Lu Ma Me Je Ve Sa Di
E             ?         ^  ^  ^  ^  ^  ^  ^
E             - déc.  28 29 30  1  2  3  4
E             ?        --
E             + déc 28 29 30  1  2  3  4 ...
E
E             ...Full output truncated (40 lines hidden), use '-vv' to show

tests/cal_display_test.py:391: AssertionError
=========================== short test summary info ============================
FAILED tests/cal_display_test.py::test_vertical_month_unicode_weekdeays_gr - ...
FAILED tests/cal_display_test.py::test_vertical_month_abbr_fr - AssertionErro...
============= 2 failed, 285 passed, 2 skipped, 2 xpassed in 7.02s ==============

Copy link
Contributor

@DamienCassou DamienCassou left a comment

Choose a reason for hiding this comment

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

Thank you for your work. When I read the changes made by the commit in this PR, I see:

  • stable.nix was deleted and vdirsyncerStable is deprecated;
  • default.nix is changed significantly.

I think the story your commits should tell is something like:

  • what was in default.nix is deleted
  • stable.nix becomes the new default

To tell this story, I think I would decompose the PR into 2 commits:

  1. The first commit deletes default.nix and makes vdirsyncer points to stable.nix
  2. The second commit renames stable.nix into default.nix

The deprecation of vdirsyncerStable can happen in any of these 2 commits or a third one. I would put it in a third commit because I like small and atomic commits but that's up to you.

@magnetophon
Copy link
Member Author

@DamienCassou Like this?

pkgs/top-level/aliases.nix Outdated Show resolved Hide resolved
@DamienCassou
Copy link
Contributor

Like this?

Exactly, great job. This is so much easier to understand, thank you. I would only add a sentence to the first commit explaining why the file is deleted.

@magnetophon
Copy link
Member Author

I would only add a sentence to the first commit explaining why the file is deleted.

How would you word it?

@ofborg ofborg bot requested a review from loewenheim November 8, 2020 11:01
@DamienCassou
Copy link
Contributor

I would only add a sentence to the first commit explaining why the file is deleted.
How would you word it?

vdirsyncer: delete default.nix, point vdirsyncer to stable.nix

TLDR: default.nix was pointing to an unmaintained code base whereas stable.nix is up-to-date and maintained.

History
1. At first their was one Python version of vdirsyncer that had been working fine for years. Then, maintenance decreased and the package was marked as broken in nixpkgs.
2. The original author (@untitaker on github.com) of vdirsyncer decided to re-implement (part of) vdirsyncer in Rust. Nixpkgs made `vdirsyncer` point to the Rust version and renamed the Python historical version to `vdirsyncerStable`.
3. Eventually, @untitaker gave up on the Rust version.
4. Someone else (@WhyNotHugo on github.com) decided to take over maintenance of the Python version.
5.  Mario Rodas (@marsam on github) and Damien Cassou updated the `vdirsyncerStable` to point to the work of @WhyNotHugo and mark the package as working again.

TLDR: default.nix was pointing to an unmaintained code base whereas stable.nix is up-to-date and maintained.

History
1. At first their was one Python version of vdirsyncer that had been working fine for years. Then, maintenance decreased and the package was marked as broken in nixpkgs.
2. The original author (@untitaker on github.com) of vdirsyncer decided to re-implement (part of) vdirsyncer in Rust. Nixpkgs made `vdirsyncer` point to the Rust version and renamed the Python historical version to `vdirsyncerStable`.
3. Eventually, @untitaker gave up on the Rust version.
4. Someone else (@WhyNotHugo on github.com) decided to take over maintenance of the Python version.
5.  Mario Rodas (@marsam on github) and Damien Cassou updated the `vdirsyncerStable` to point to the work of @WhyNotHugo and mark the package as working again.
@magnetophon
Copy link
Member Author

@DamienCassou Thanks for the text!

@ofborg ofborg bot requested a review from loewenheim November 8, 2020 13:32
@magnetophon magnetophon force-pushed the vdirsyncer branch 2 times, most recently from 1bf3f89 to 6069aaa Compare November 9, 2020 08:35
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Nov 9, 2020

Result of nixpkgs-review pr 103073 run on x86_64-darwin 1

1 package failed to build:
  • khal
2 packages built:
  • python37Packages.vdirsyncer
  • vdirsyncer (python38Packages.vdirsyncer)

khal

>           assert strip_accents('\n'.join([line.lower() for line in vert_str])) == \
                '\n'.join(example_gr)
E               AssertionError: assert '\x1b[1m    δ...  1  2  3  4 ' == '\x1b[1m     ...  1  2  3  4 '
E                 -      δε τρ τε πε πα σα κυ
E                 ?         -
E                 +     δε τρ τε πε πα σα κυ
E                 - δεκ  28 29 30  1  2  3  4
E                 ?         -
E                 + δεκ 28 29 30  1  2  3  4
E                 -       5  6  7  8  9 10 11 ...
E
E                 ...Full output truncated (39 lines hidden), use '-vv' to show

tests/cal_display_test.py:368: AssertionError
_________________________ test_vertical_month_abbr_fr __________________________

    def test_vertical_month_abbr_fr():
        # see issue #653
        try:
            locale.setlocale(locale.LC_ALL, 'fr_FR.UTF-8')
            vert_str = vertical_month(month=12, year=2011,
                                      today=dt.date(2011, 12, 12))
>           assert '\n'.join(vert_str) == '\n'.join(example_fr)
E           AssertionError: assert '\x1b[1m    L...  1  2  3  4 ' == '\x1b[1m     ...  1  2  3  4 '
E             -       lu ma me je ve sa di
E             ?         ^^^  ^  ^  ^  ^  ^  ^
E             +     Lu Ma Me Je Ve Sa Di
E             ?         ^  ^  ^  ^  ^  ^  ^
E             - déc.  28 29 30  1  2  3  4
E             ?        --
E             + déc 28 29 30  1  2  3  4 ...
E
E             ...Full output truncated (40 lines hidden), use '-vv' to show

tests/cal_display_test.py:391: AssertionError
=========================== short test summary info ============================
FAILED tests/cal_display_test.py::test_vertical_month_unicode_weekdeays_gr - ...
FAILED tests/cal_display_test.py::test_vertical_month_abbr_fr - AssertionErro...
============= 2 failed, 285 passed, 2 skipped, 2 xpassed in 6.39s ==============

@peterhoeg
Copy link
Member

All good - tested here!

@peterhoeg peterhoeg merged commit 89fae7b into NixOS:master Nov 16, 2020
@magnetophon magnetophon deleted the vdirsyncer branch November 16, 2020 13:49
@wirew0rm
Copy link
Contributor

This seems to have broken the hydra build (and it won't build for me locally either)?

First evaluation after the merge broke, subsequent evaluations use the cached failure:
https://hydra.nixos.org/build/130683519#tabs-summary

Looking at the log, the error seems to be the same as the one I'm seeing locally and which was also discussed previously in this nixpkgs PR and this upstream Issue.

If I locally disable the flaky test, it works for me locally:

--- a/pkgs/development/python-modules/vdirsyncer/default.nix
+++ b/pkgs/development/python-modules/vdirsyncer/default.nix
@@ -53,7 +53,10 @@ buildPythonPackage rec {
-  disabledTests = [ "test_verbosity" ];
+  disabledTests = [
+    "test_verbosity" # was always disabled
+    "test_create_collections" # flaky test sometimes exceeds its deadline
+  ];

@jonringer
Copy link
Contributor

@wirew0rm if would like to make a PR, I'll be happy to review it

@wirew0rm wirew0rm mentioned this pull request Nov 28, 2020
10 tasks
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.

8 participants