-
-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
transmission: sent a warning and alias it to transmission_3 #258058
Conversation
This looks like |
ad088df
to
dfb3166
Compare
Why do you consider it as "data loss"? It's not files that were already downloaded are deleted from your storage, but rather it is the "torrent configuration" loss. I personally don't think it is such a bad bug to live with. |
Yes, it loses torrent configuration in a way that downgrading does not restore. This is quite painful if you have large amount of affected torrents that you have configured. Thankfully, I had backups or I would be very annoyed. |
Hmm I understand... Your usage of transmission is unique IMO. Perhaps we can add some release notes, with links to upstream bug reports? My personal opinion is that unless upstream released a completely broken package, a distribution shouldn't hold such significant updates from all users just because some users experience a bug that can barely be reproduced... I wasn't aware of this upstream release if I hadn't noticed it by chance by browsing Nixpkgs' pull requests. BTW I don't understand what is the |
dfb3166
to
6247609
Compare
I'm attempting to fix the darwin build of transmission 4. I pushed several other improvements to both expressions, along with a release-notes line for the possible upstream bug experience. |
@ofborg build transmission |
The darwin build seems fixed. |
I've been using 4.0.3 for a while, and lately ran into an issue where data was being downloaded, but not saved to disk. The torrents showed multiple GBs downloaded but still 0% progress. This was happening with all downloading torrents, even newly added ones, and was fixed by restarting Transmission. So even though I'm using v4 I'm against making it the default as in my experience it's not yet as stable as v3. Your goal seems to be to raise awareness about v4. I can think of a couple ways to do that without risking people's data:
In both cases, we should explain why v4 isn't the default and link the PRs/upstream bug so users can make an informed decision. |
Thanks @ThinkChaos for the ideas. I like them overall. As for:
Were you thinking of something like this in transmission = throw "Please instead of attribute `transmission`, use `transmission_3` or `transmission_4`"; vs: transmission = lib.warn "The transmission attribute without version suffix is deprecated, please switch over to either transmission_4 or transmission_3. Note the release-notes mentioning upstream bugs." transmission_3; Or simply (which would not make a difference for most users, unless aliases are disabled in their transmission = transmission_3; I'd appreciate your vote on either of these options. Moreover, if you'd report that with 4.0.4, you don't experience the buggy behavior you observed with 4.0.3, that should affect this decision. |
sorry for noise
|
See: https://noogle.dev/?selected=%22lib.trivial.warn%22&term=%22warn%22 |
@doronbehar thank you for you response |
I would prefer
I do not think many people running unstable read release notes. Personally, I skim the commit log but it is really easy to miss something important there. As such, I would not consider a mention in release notes to be a sufficient precaution for bumping the There is a section for announcing new versions and Transmission 4 definitely deserves a mention (with a warning) but that just would not be enough for the bump IMO.
I would consider any data loss a serious breakage. I would not be against bumping the attribute if it was just the feature that was broken but it clears the torrent status so it is not possible to fix by downgrading once user ran 4.0. And the bug can be reproduced easily – just because it is a niche feature that does not affect many torrent files does not mean it does not seriously affect many peopleʼs workflows. |
I would personally lean more towards
I think you're right, personally I've started diffing the next release notes md file when updating my flake, works great :)
I agree, I meant just as a way to say " |
I opened a PR that does everything done in this file besides the rename, so that the discussion here will stay focused: #259361 . Your review is welcome.
@jtojnar I feel we are in almost full agreement. Could you perhaps voice perhaps a more concrete opinion on the options suggested in my previous comment above? |
1a0d1e5
to
033ddce
Compare
033ddce
to
f488a58
Compare
I see 4.00 still has some issues.
I think you should still cherry pick my changes. |
Maybe we should create a thread in Discourse Announcements category like the following, and link it in the error message: https://pad.lassul.us/lHdWr37LRxS-8gEHYWvfuA?both (Feel free to edit)
I think this is not our responsibility. And doing so might instead cause confusion for people switching from other distribution to NixOS. At most, we can mention it in the announcement. |
It looks like a nice use case for RFC 127 problem infrastructure! https://github.com/NixOS/rfcs/blob/master/rfcs%2F0127-issues-warnings.md |
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.
There is another reference to pkgs.transmission
in nixosTests.transmission
:
nixpkgs/nixos/tests/all-tests.nix
Line 980 in c94ae9b
transmission = handleTest ./transmission.nix { transmission = pkgs.transmission; }; |
Thanks for the ideas @jtojnar . I added both a notice to the upcoming release notes, based on the text you wrote here.
I agree. I'll write a comment at the discourse thread when this is merged. |
f488a58
to
ecacf3c
Compare
ecacf3c
to
f3e525d
Compare
I ran into another v4 bug that's technically data loss, sharing here for awareness: Downloaded data not saved, might be nice to also add to the release notes. |
f3e525d
to
63cd062
Compare
Linked to it in the release notes 👍. |
So.. @jtojnar do I get your final approval on this change? |
63cd062
to
8ce56cc
Compare
8ce56cc
to
7645dde
Compare
Once again I fixed the merge conflicts. Since the last revision of this PR, I got only approvals and hadn't seen any objections to it. Hence I'll merge it myself in a day or two. |
Motivation for this change
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes