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

google-music-scripts: fix build #64221

Merged
merged 2 commits into from Jul 4, 2019
Merged

google-music-scripts: fix build #64221

merged 2 commits into from Jul 4, 2019

Conversation

jbaum98
Copy link
Contributor

@jbaum98 jbaum98 commented Jul 3, 2019

Motivation for this change

This will be necessary to fix google-music-scripts.

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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@jbaum98 jbaum98 requested a review from FRidh as a code owner July 3, 2019 02:22
@jbaum98 jbaum98 changed the title Loguru python3Packages.loguru: init at 0.3.0 Jul 3, 2019
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.

I would move the disable logic into the nix expression, rather than in the top-level listing

pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/loguru/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/loguru/default.nix Outdated Show resolved Hide resolved
@jbaum98
Copy link
Contributor Author

jbaum98 commented Jul 3, 2019

I just realized google-music-scripts needs an older version, so this PR is kind of moot. Is it worth merging if no packages will use it?

@jonringer
Copy link
Contributor

I would probably shelf it, not sure.

The most "proper" thing to do would be to upstream the dependency change. Then package it :) But that takes a fair amount of time depending on the size of the library.

@jbaum98
Copy link
Contributor Author

jbaum98 commented Jul 3, 2019

Turns out it just runs with the change. So I think we should merge this and patch google-music-scripts.

@jonringer
Copy link
Contributor

If you're creating this package to enable the google-music-scripts package, i would just combine the two and use the same PR (but with 2 commits, one for each package)

@jbaum98
Copy link
Contributor Author

jbaum98 commented Jul 3, 2019

Sounds good.

@jbaum98 jbaum98 changed the title python3Packages.loguru: init at 0.3.0 Fix google-music-scripts Jul 3, 2019
@jbaum98 jbaum98 force-pushed the loguru branch 2 times, most recently from 36d3ad0 to b186964 Compare July 3, 2019 03:04
@jonringer
Copy link
Contributor

jonringer commented Jul 3, 2019

for the title of the PR, I would probably go with google-music-scripts: fix build, like you have in your commit history :)

@jbaum98
Copy link
Contributor Author

jbaum98 commented Jul 3, 2019

Very fair.

@jbaum98 jbaum98 changed the title Fix google-music-scripts google-music-scripts: fix build Jul 3, 2019
@jonringer
Copy link
Contributor

.. now i feel terrible, pythonPackages.google-music-scripts: fix build, that way the ofBorg bot is able to build the package that is listed prior to the ':'

https://github.com/NixOS/ofborg#automatic-building

@jbaum98
Copy link
Contributor Author

jbaum98 commented Jul 3, 2019

.. now i feel terrible, pythonPackages.google-music-scripts: fix build, that way the ofBorg bot is able to build the package that is listed prior to the ':'

https://github.com/NixOS/ofborg#automatic-building

google-music-scripts isn't a package, it's an application, so I think this is correct. Unless I've misunderstood?

@jonringer
Copy link
Contributor

oh yea, you're right :), my bad about being bad.

@worldofpeace
Copy link
Contributor

I'm seeing an issue with the tests passing for python37Packages.loguru locally.

Test failures
=================================== FAILURES ===================================
__________________ test_time_rotation_reopening_native[False] __________________

tmpdir_local = PosixPath('tmpxkipvoap'), delay = False

    @pytest.mark.parametrize("delay", [False, True])
    def test_time_rotation_reopening_native(tmpdir_local, delay):
        filepath = str(tmpdir_local / "test.log")
        i = logger.add(filepath, format="{message}", delay=delay, rotation="1 s")
        logger.info("1")
        time.sleep(0.75)
        logger.info("2")
        logger.remove(i)
        i = logger.add(filepath, format="{message}", delay=delay, rotation="1 s")
        logger.info("3")

        assert len(list(tmpdir_local.iterdir())) == 1
        assert (tmpdir_local / "test.log").read_text() == "1\n2\n3\n"

        time.sleep(0.5)
        logger.info("4")

>       assert len(list(tmpdir_local.iterdir())) == 2
E       AssertionError: assert 1 == 2
E        +  where 1 = len([PosixPath('tmpxkipvoap/test.log')])
E        +    where [PosixPath('tmpxkipvoap/test.log')] = list(<generator object Path.iterdir at 0x7ffff520e318>)
E        +      where <generator object Path.iterdir at 0x7ffff520e318> = <bound method Path.iterdir of PosixPath('tmpxkipvoap')>()
E        +        where <bound method Path.iterdir of PosixPath('tmpxkipvoap')> = PosixPath('tmpxkipvoap').iterdir

tests/test_filesink_rotation.py:281: AssertionError
__________________ test_time_rotation_reopening_native[True] ___________________

tmpdir_local = PosixPath('tmpc0qhz2we'), delay = True

    @pytest.mark.parametrize("delay", [False, True])
    def test_time_rotation_reopening_native(tmpdir_local, delay):
        filepath = str(tmpdir_local / "test.log")
        i = logger.add(filepath, format="{message}", delay=delay, rotation="1 s")
        logger.info("1")
        time.sleep(0.75)
        logger.info("2")
        logger.remove(i)
        i = logger.add(filepath, format="{message}", delay=delay, rotation="1 s")
        logger.info("3")

        assert len(list(tmpdir_local.iterdir())) == 1
        assert (tmpdir_local / "test.log").read_text() == "1\n2\n3\n"

        time.sleep(0.5)
        logger.info("4")

>       assert len(list(tmpdir_local.iterdir())) == 2
E       AssertionError: assert 1 == 2
E        +  where 1 = len([PosixPath('tmpc0qhz2we/test.log')])
E        +    where [PosixPath('tmpc0qhz2we/test.log')] = list(<generator object Path.iterdir at 0x7ffff5ceab10>)
E        +      where <generator object Path.iterdir at 0x7ffff5ceab10> = <bound method Path.iterdir of PosixPath('tmpc0qhz2we')>()
E        +        where <bound method Path.iterdir of PosixPath('tmpc0qhz2we')> = PosixPath('tmpc0qhz2we').iterdir

tests/test_filesink_rotation.py:281: AssertionError

@jbaum98
Copy link
Contributor Author

jbaum98 commented Jul 3, 2019

What platform are you on? I ran it again and it passes for me.

@worldofpeace
Copy link
Contributor

I always test against NixOS with sandboxing enabled.

I'm half guessing (because the mention of paths) that it's related to the Nix sandbox.

@jbaum98
Copy link
Contributor Author

jbaum98 commented Jul 3, 2019

@worldofpeace Although I couldn't reproduce the issue even with sandboxing enabled, I think the patch I just pushed might fix the problem. It still passes on my machine, but that doesn't mean much. Can you give it a try?

@worldofpeace
Copy link
Contributor

Still fails ☹️

Log
=================================== FAILURES ===================================
__________________ test_time_rotation_reopening_native[False] __________________

tmpdir_local = PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n0')
delay = False

    @pytest.mark.parametrize("delay", [False, True])
    def test_time_rotation_reopening_native(tmpdir_local, delay):
        filepath = str(tmpdir_local / "test.log")
        i = logger.add(filepath, format="{message}", delay=delay, rotation="1 s")
        logger.info("1")
        time.sleep(0.75)
        logger.info("2")
        logger.remove(i)
        i = logger.add(filepath, format="{message}", delay=delay, rotation="1 s")
        logger.info("3")

        assert len(list(tmpdir_local.iterdir())) == 1
        assert (tmpdir_local / "test.log").read_text() == "1\n2\n3\n"

        time.sleep(0.5)
        logger.info("4")

>       assert len(list(tmpdir_local.iterdir())) == 2
E       AssertionError: assert 1 == 2
E        +  where 1 = len([PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n0/test.log')])
E        +    where [PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n0/test.log')] = list(<generator object Path.iterdir at 0x7ffff5210390>)
E        +      where <generator object Path.iterdir at 0x7ffff5210390> = <bound method Path.iterdir of PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n0')>()
E        +        where <bound method Path.iterdir of PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n0')> = PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n0').iterdir

tests/test_filesink_rotation.py:280: AssertionError
__________________ test_time_rotation_reopening_native[True] ___________________

tmpdir_local = PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n1')
delay = True

    @pytest.mark.parametrize("delay", [False, True])
    def test_time_rotation_reopening_native(tmpdir_local, delay):
        filepath = str(tmpdir_local / "test.log")
        i = logger.add(filepath, format="{message}", delay=delay, rotation="1 s")
        logger.info("1")
        time.sleep(0.75)
        logger.info("2")
        logger.remove(i)
        i = logger.add(filepath, format="{message}", delay=delay, rotation="1 s")
        logger.info("3")

        assert len(list(tmpdir_local.iterdir())) == 1
        assert (tmpdir_local / "test.log").read_text() == "1\n2\n3\n"

        time.sleep(0.5)
        logger.info("4")

>       assert len(list(tmpdir_local.iterdir())) == 2
E       AssertionError: assert 1 == 2
E        +  where 1 = len([PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n1/test.log')])
E        +    where [PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n1/test.log')] = list(<generator object Path.iterdir at 0x7ffff5be3a98>)
E        +      where <generator object Path.iterdir at 0x7ffff5be3a98> = <bound method Path.iterdir of PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n1')>()
E        +        where <bound method Path.iterdir of PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n1')> = PosixPath('/build/pytest-of-nixbld/pytest-0/test_time_rotation_reopening_n1').iterdir

tests/test_filesink_rotation.py:280: AssertionError

@jbaum98
Copy link
Contributor Author

jbaum98 commented Jul 3, 2019

I don't think it has to do with the paths, I think it has to do with time. This test is testing the feature where it automatically rotates logs after a given duration. Does that sound right? If that's the case, we probably just want to skip this test right?

@jbaum98
Copy link
Contributor Author

jbaum98 commented Jul 3, 2019

In case you agree with the above, I added a commit disabling that test.

@worldofpeace
Copy link
Contributor

worldofpeace commented Jul 4, 2019

Yeah that sounds impure.

Also you need to drop the pytest-mock commit because it's also been fixed on master

@jbaum98
Copy link
Contributor Author

jbaum98 commented Jul 4, 2019

As a side question, is there a way for me to be pinged next time a package I'm a maintainer on breaks? I only noticed these packages were broken when I was pinged from the semi-automatic upgrade PRs.

@worldofpeace
Copy link
Contributor

As a side question, is there a way for me to be pinged next time a package I'm a maintainer on breaks? I only noticed these packages were broken when I was pinged from the semi-automatic upgrade PRs.

I think Hydra used to be enabled to send emails but I think that's disabled now.
So currently the update PR's would be the only way to have your attention requested or another
contributor who notices.

Does seem that dotlambda notified you though 12d625f#commitcomment-32771166 but that's was a while ago now.

@worldofpeace worldofpeace merged commit dfb1109 into NixOS:master Jul 4, 2019
@jbaum98
Copy link
Contributor Author

jbaum98 commented Jul 4, 2019

Oh yup, that's on me.

Thanks @worldofpeace!

@jbaum98 jbaum98 deleted the loguru branch July 4, 2019 09:37
@worldofpeace
Copy link
Contributor

backported in 2a0ca05 2eea9b3

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.

None yet

3 participants