nixos/qbittorrent-nox: init#337109
nixos/qbittorrent-nox: init#337109Redhawk18 wants to merge 1 commit intoNixOS:masterfrom Redhawk18:master
Conversation
There was a problem hiding this comment.
If I remember correctly, qbittorrent-nox will want you to accept some kind of legal agreement or it won't start. How is this supposed to be handled?
In my module I can just set services.qbittorrent.settings.LegalNotice.Accepted = true;, something like this would be great. There could also be an assert like this.
|
Is the uid and gid okay? I wasn't sure if there's a way to generate it dynamically. |
I believe the desktop app does, But I installed this to test it on my desktop and I was never prompted once to agree to it. It's likely a bug on their end, and even if we added an assert the eula would still show up. It's not like with minecraft where you could write |
|
@NixOS/nix-formatting The error log told me to ping you with errors. |
You need to run your module on a fresh machine. A NixOS test would actually be great way to make sure it works.
Why not? Adding this to the It's exactly what the module I linked does.
The error log says this:
|
|
I read the error message lol, I asked because I did what it suggested and it still showed an error, but it looks like it can't find the file to begin with. |
Will this override the config even if the settings option is not set? |
Not sure what you mean, it works fine: |
Ah I ran |
|
Are the |
Looks like the error message could be a bit clearer still: NixOS#337109 (comment)
|
Improving the message to hopefully prevent such confusion with #337577 😄 |
Hey I did some testing on this idea and it's insanely buggy and hard to get working correctly. I'm likely going to remove the ability to add settings. There's also no docs explaining the options, so it leads me to believe that the qbittorrent team views this as an internal config file. Which means they will change how data is structured without warning which would mean all settings set by us would no longer work and there's no declarative way to really stop this. |
They are shown here https://nixos.org/manual/nixos/unstable/ |
|
I think this pr is different enough, that other pr seems very focused on all the details of a declarative system and is inactive, this is just the ground work for this service. We can always submit a new pr with those options later. |
misuzu
left a comment
There was a problem hiding this comment.
It works! Added some suggestions for the documentation
|
Right, you should also add a release notes entry for your module. |
bbigras
left a comment
There was a problem hiding this comment.
It runs fine :)
Thank you very much for the module.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4514 |
There was a problem hiding this comment.
The port option is going to be ambiguous when someone adds an option for --torrenting-port.
In general, I think #287923 is in a slightly better shape (with declarative config mostly done, and hardening options on the service). I would rather we merge the parts that are already working from that PR rather than deviate more.
I highly disagree, and blocking my pr seems incredibility hostile to me. Like I already said in the other pr @fsnkty admitted they are unable to work on this and the pr is incomplete. I rather have a working service that doesn't have all the options it possibility could than a pr that has been open for years that is almost perfect. My pr is finished and already has 4 reviews, blocking it at this point seems like whoever was first gets to merge their pr. Not whoever has the better and more active pr with more approval. |
|
@Redhawk18 I'm not blocking your PR, I'm pointing out that both PRs are to add essentially the same module. I did see that @fsnkty is not currently able to work on their PR, and I do appreciate you wanting to get something merged in. But from comparing both PRs, I think yours is diverging from the first one for no reason that I can see, and I would rather we just have the same basis as I think the first PR had more reviews done to it and resolved some of those same pain points. I said:
This might have been phrased badly, but I hope my answer cleared up than I'm not blocking this PR. I do want to see it be merged, but I voiced what I think is wrong in my review (namely, the naming of the module, the |
use tmpfiles instead of prescript suggestions fmt remove mddoc refmt doc strings add ability to declearive declear config add description for settings option fmt fmt add doc and remove settings rename file attempt to rebuild docs correctly fmt suggestions 3 doc suggestions doc suggestion 2 doc suggestion 3 typo doc suggestion 4 suggestions 4 fmt suggestions 5 wants suggestion 6 fmt rename port and add hardening fmt
Fine, I renamed the port and added the hardening, but I'm not renaming the module. If you can prove qbittorrent can run over xorg which is on life support first off and work on wayland I will rename the module. The name is correct how it is now because it only uses nox, so it should be named after it. |
nothings personal or being blocked.
it needs testing and some minor fixup, it's been in use by multiple people for months now including myself, qbit is a bizarre program and I simply didn't want to have the module included just for it to break in bizarre ways due to some qbit feature I was unaware of.
take what works 100% then, no sense in nearly duplicating work and rerequesting reviews, it also creates more work for anyone who now has to work around and refine the module you've made, or argue for a change in it's config
not being blocked, just please be cautious of the extra work this causes for yourself and for anyone else who wants to work on the module as it'd now make extra changes. |
I still believe blocking my pr over a minor name change that implies this service works with both the ui and headless version of qbittorrent is a bad idea.
I had no idea until the pr had progressed far enough to be reviewed. It seems my pr has made your pr more active as well by the threat of being closer to merging. And I understand you don't want to edit my code, and rebase your pr which I understand. I'd also rather write everything myself aswell. I made this pr because I wanted this service and you were inactive, I understand you can't choose when you have time in your life for free unpaid volunteer work that gets no credit anywhere. But I believe I have been incredibility reasonable with all suggestions, and everything was progressing smoothly until now. The reason a block offenses me so much is the amount of work I've put into this for someone on the internet to just click a reject button with a bad suggestion, and prevent this from being merged is insane to me. |
it's not about my time, the differences here seem unnecessary, it's not about wanting to avoid rebasing. |
What do you want to call a suggestion I strongly disagree with, that blocks my entire pr and makes my work a waste of time? I came up with this pr by myself as something I wanted to improve to the nix ecosystem. There's no diverging of work, just because you made the pr first doesn't mean my work is a derivative. I understand I'm the bad guy because I got angry first, but maybe you would understand if you put hours into something just for someone to click block without even testing or checking out the code, or a thought into what the changes they are suggesting mean. I'm not even sure I want to contribute this pr anymore this whole process has been so terrible since I left that comment on your pr. |
some stronger justification for why you disagree with it, it's not a personal thing.
they aim for the same thing with different scopes, you diverge from existing work, no intentions to imply it's a derivative though I have suggested that maybe it should be in some ways.
these are some rude assumptions and don't invite collaboration :(
if your goal truely is to have the basics of the module included in nixpkgs faster, rather than focusing on the few blockers on the rest of it, that's fine! it's always nice to get stuff started and to get more work included. |
Because by the time I saw it a majority of it is already implemented, I'm not sure about the hardening still I would have to do a lot of testing to see if it even works well enough to include without breaking features in unexpected ways. Also are you sure you can still have an undeclarative setup if thats what the user desires or is that impossible with your pr? Can you blame me for thinking your pr was dead, there was over a month with no comments or new pushes? |
not the kinda justification I'm looking for, that's very personal and a "but I did this work" nothing to do with why it is or isn't better than existing work.
testing is a requirement, you shouldn't want to include work you're unsure of, you also shouldn't want to include work you're unsure about the safety of.
not using the option is an option yeah, this discussion belongs on the other PR
currently just do not have access to the hardware required to test or work on it 😕 not really justification to dismiss it for consideration in conversation when having your work reviewed remains unproductive, sorry I'll be dismissing this, best of luck! |
My pr is better because I am active and ready to take suggestions and make changes to merge this. You are projecting your reasons you like your pr more onto me. |
|
@ambroisie Are you willing to change your mind about the name change before I do testing on the hardening? I rather ask now and close this than waste more of my time on a pr that won't get merged because it wasn't first. |
|
@fsnkty blocking me is rude after telling me my work on this pr has no value. I will be talking to the nixos owners team about this and your pr. |
|
@Redhawk18 I think you need to calm down a little. The other PR shows still some activity and the author has shown a good deal of patience to communicate to you what are the expectations of the PR and what steps needs to be done to carry that over to the finish line. While I appreciate your motivation and energy to carry this over the finish line, just actively demanding peoples attention in an open source project is bad netiquette. We are a big project with a lot of contributors and sadly sometimes changes take time and cooperation to get into the right shape. NixOS gives you a lot of tools to locally test and develop changes without relying on upstream changes so immediate action is seldom necessary. |
I blocked you after struggling too much to take the conversation somewhere productive, you're personal attacks to the expectations and assumptions of others and myself make me uncomfortable working with you. if you dislike me and my comments you have every opportunity to dismiss me. |
|
To clarify, @Lassulus response is on behalf of the so-called owners. We are the moderation team and we stand behind this message. |
My mistake, I asked on the nixos discord who can I talk to and they told me the nixos org owners, not the mod team. |
Thank you for responding, its my mistake for expecting things to move as fast as they do at my job, this is a totally different environment. That was just how I've been use to doing things. In my head the thought of creating a well maintained and well developed module for a service shouldn't belong in my config alone, that's why I wanted to have a pr. I would like to extend an olive branch to @fsnkty I believe both of us felt attacked by the other pr, I know I did. I reported you to the mod team because I felt this was a destructive road this was going down. I would appreciate it if you would accept that trying to merge my pr is in anyway hostile. Also I would like to add how my personal feelings tolds you do not matter in the context of pull requests, you can have coworkers you strongly dislike. However you still have to work as a team and work out or work around the differences you might have. I would like to close this pr if you unblock me and I can add some of the features I like from this pr to yours as suggestions. I believe this is a solution that would make everyone happy. |
thankfully in this environment you are not made to deal with all co contributors, I see no conflict to resolve as you are as free to work on this as you always were, I am in no way able or even willing to block you from doing so. I cant manage to word my concerns properly so I'll unfortunately leave it at that for now and deal with any potential future issues whenever I'm actually able to lol. best of luck, qbit is bizarre software. |
Communication skills are more important than any technical skill you could ever obtain, my pr is an example of that. My best wishes to your pr I hope you can make it before 30.05 releases. |
Looks like the error message could be a bit clearer still: NixOS#337109 (comment) (cherry picked from commit 249d4a9)
use tmpfiles instead of prescript
Description of changes
This adds a service for the qbittorrent nox web ui with some basic options for the systemd job.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.