-
-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
Easy to check to be unquestionably good changes #6968
Conversation
My understanding is that using |
That's a pity because
would configure all the packages that support this option. Much better than overrides IMHO. |
c) |
|
||
buildInputs = [ lua scons ]; | ||
|
||
patches = [ /home/oxij/src/toluapp/oxij.patch ]; |
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.
Add patch and fix this path?
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.
Oops, yep :)
So 👍 for this except for the notes. And don't mix this PR with new packages or package updates. Please issue a different PR for each new package or package update. |
@peti config is a very good way to say: "I don't want packages to be compiled with x11 support, globally". I wonder how would you do that without config. |
@lethalman, yes, that is true. I agree there are legitimate use cases for |
@lethalman I was under the same impression as @peti, the |
@codyopel agreed there, that's why we have package overrides for. But this PR doesn't add them, it's only nitpicking. |
@lethalman Git is not |
8f937c4
to
ae0d47a
Compare
Okay. I cut the tail of this. Now there are only fixes here. Updates and additions will go into another PR. |
Now that this became a fixes-only patch let me put this here too. |
The practice of mixing up half a dozen unrelated patches in one PR really makes a mess of things. There's is too much stuff going on in this PR, really. This should be split up into multiple PRs that deal with one subject / package each. |
242563a
to
05bf782
Compare
@peti They are related. Now that all the meaningful changes were rejected as too complicated, all they do is transform |
I will test a couple of things but otherwise looks fine to me. |
Also I suspect that 1 PR is easier to check than 14 (this PR) and ~40 more (in queue). |
No, it's not. On 25 March 2015 20:34:37 Jan Malakhovski notifications@github.com wrote:
|
@@ -11433,7 +11420,7 @@ let | |||
|
|||
eiskaltdcpp = callPackage ../applications/networking/p2p/eiskaltdcpp { lua5 = lua5_1; }; | |||
|
|||
qemu = callPackage ../applications/virtualization/qemu { }; | |||
qemu = callPackage ../applications/virtualization/qemu (config.qemu or {}); |
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.
IMHO, this functionality duplicates the packageOverrides
mechanism.
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 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.
No sorry but I agree with @peti here, I thought there was already a config.qemu, not that you added a new one. This is something you can do with packageOverrides
. Don't add this please.
And apparently same for everything else, I really misread this PR by thinking there was already a config.foo.
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.
Okay. Let's talk about this. For me this "there are packageOverrides" sounds like. "Why would you need NixOS when you could slap this bunch of packages together and then configure all of them by hand". I can't see why having a readable mechanism for setting default values is a bad thing.
If what you are actually want to say is that this looks ugly, then okay, a valid point. But that is trivially fixed by adding a version of callPackage
that does this stuff automatically (this patch is in queue). After which I'd say that it is packageOverrides
that looks ugly.
Finally, I would like to get NixOS-like configuration for packages' flags. Because currently with package overrides it's very easy to get several versions of the same package when, in fact, it would be sufficient to have only one. Do egrep -rn '\.override' nixos/modules
, some of these will actually compile another version when all they need is a flag to be turned on.
To solve this one could make NixOS override packages globally, but this will eventually result in a confusion when user overrides a package and then NixOS overrides it too and who knows what will the options actually be set to.
Note, I'm not saying that overriding doesn't have it uses, it's most certainly does (and some of the uses in nixos/modules
are pretty valid). I'm saying that you probably don't want NixOS modules to override packages globally, but I can see when and why NixOS modules may want to change the defaults (see, e.g. #6969).
That was an expansion of #6968 (comment) a), there is also #6968 (comment) b) which is about setting common options (a patch for which is, again, in a queue) and, finally, #6968 (comment)
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.
Also, /cc @edolstra on this.
@oxij, all I know is that I would have merged oxij@1901343 within seconds without batting an eye. Unfortunately, I can't do that because the commit is mixed up with lots of other changes. The difference is that now I have to feel comfortable merging all of them before I can press that button. If these commits had been submitted separately, then it would be easier for collaborators to cherry-pick those changes. Also, what I meant to convey with the term "unrelated" is that these patches for the most part don't depend on another. There is no dependency between them in the sense that B cannot be merged unless A has been merged first, etc. If that were the case, then submitting A and B together might be a good idea. In this PR, however, most of the commits stand on their own and can be merged independently of each other. |
@peti okay, that is a valid point. But as a submitter I'd say that with that many changes it is far more work to split them into separate branches and test them (because then I would have to split my tests into branches), and if I still want to use these changes myself (I do) I would need either merge all these split branches together generating a huge bloated history and then rebase the next set of dependent patches on top and repeat (the only idea of this gives me chills), or maintain all these branches separately from my own master, which would probably be more manageable (except it would need far more |
… would hopefully stop breaking the older kernels
05bf782
to
4cd43bd
Compare
@peti you win, I removed everything debatable from here. I hope now it is trivial enough. Everything else will go into their own PRs. |
Easy to check to be unquestionably good changes
According to #7045, |
No description provided.