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
Conversation
There was a problem hiding this 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
I just realized |
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. |
Turns out it just runs with the change. So I think we should merge this and patch |
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) |
Sounds good. |
36d3ad0
to
b186964
Compare
for the title of the PR, I would probably go with |
Very fair. |
.. now i feel terrible, |
|
oh yea, you're right :), my bad about being bad. |
I'm seeing an issue with the tests passing for Test failures
|
What platform are you on? I ran it again and it passes for me. |
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. |
@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? |
Still fails Log
|
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? |
In case you agree with the above, I added a commit disabling that test. |
Yeah that sounds impure. Also you need to drop the |
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. Does seem that dotlambda notified you though 12d625f#commitcomment-32771166 but that's was a while ago now. |
Oh yup, that's on me. Thanks @worldofpeace! |
Motivation for this change
This will be necessary to fix google-music-scripts.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)