Fully declarative modules#2563
Conversation
|
Using this for several deployments and all is behaving as expected so far. |
|
Rebased and built on top of |
This makes the relationship between property types clearer, and more importantly will let option types parameterized by other option types reuse the code for delegated type checking and merging.
This simplifies typechecking and allows properties to be used inside the lists
This simplifes typechecking and allows properties to be used inside of the attribute sets. This fixes the empty synergy-client and synergy-server services previously generated on systems with synergy disabled.
This symplifies typechecking and allows properties to be used inside the function body. It also makes possible checking the type of the result.
This allows for module arguments to be handled modularly, in particular allowing the nixpkgs module to handle the nixpkgs import internally. This creates the __internal option namespace, which should only be added to by the module system itself.
Ideally the module system could be configured pretty much completely by the contents of the modules themselves, so add comments about avoiding complicating it further and possibly removing now-redundant configurability from the existing interface.
|
This looks pretty cool. Any developments? |
|
Closing as this doesn't seem wanted. |
|
Noooooo, not another of these 😦 |
|
@shlevy I believe the whole community thinks your contributions are great. Closing the PR means it will be really forgotten. I'd please you to reopen all of them, it shouldn't cost you anything. |
|
Agreed! Same with the recursive nix one cough cough 😄 |
|
These PRs have been open for months with no feedback, and the work of keeping them in sync, tested, and otherwise ready to merge doesn't seem worth it given the lack of response no matter what I do. If I am not going to keep them up-to-date, why keep the PRs open? It's not like github will delete them, and I'm not willing to keep updating work that is never going to make it in. If interest picks up, the PR can be reopened. |
|
@shlevy the issue is just that if the PR is closed, how will interest pick up? There will be an initial few cries of sadness from the likes of me that will draw attention to the issues, and then after that they become almost impossible to discover. |
|
And if the PR is open, we have bitrotted code that no one seems to care about clogging up the already unwieldy issue list. What sign is there that interest will ever pick up? |
|
Neither of your closed PRs seemed super controversial. Could you just merge them and @rbvermaa or @edolstra can complain if they don't like it? Might put less of a bottleneck on them and allow the rest of us to reap the goodies 😄 Also, not trying to give you a hard time. I just would love to see these things you're doing make it into mainstream nix (and hnix, of course) |
|
Apparently, it's unrealistic to assume that other people can read such an extensive patch and judge the effects and consequences of applying it. Personally, I have no idea what kind of advantage that patch would give me. I don't understand the problem it solves. And I don't have time to find out. I suppose the situation is similar for other people in the community. Personally, I think the only way to get this merged is to, well, merge it. Once that stuff reaches production servers, you will get feedback. |
|
Agreed, it's not that this is going into the stable release. If something goes wrong, there's always the revert. Just merge :) |
|
For what it's worth: these seem like nice additions and would be a shame to throw away. |
There was a problem hiding this comment.
If the order is that important, perhaps a comment would be warranted. Is this picking lib from utils?
|
I think I like this but I don't fully understand it. It would be great if there were some comments in the code explaining the module evaluation flow and what The changed modules do seem a little cleaner. How about we just go for 5 👍's since there seems to be no vetoing? At 5 we merge. 👍 from me while imploring for comments. |
|
Someone else will have to pick this up if it is wanted, I'm done with it |
|
It's sad to see that nobody knows whether a merge is acceptable or not. We need to organize our project in a better way 😔 @edolstra Is there a specific reason why you didn't comment this PR yet? Is it a time related issue? If so, it would be great if we had one more person who's responsible for technical decisions |
|
What's sadder to me than this particular instance is to see talented contributors basically giving up on the entire process as a result of things like this. Agreed that we need to come up with a more formal evaluation structure, because I don't think the project can afford to lose expertise (and enthusiasm!) like @shlevy's. |
|
One possibility, given limited time/energy, is for @edolstra to tell certain people that he blanket-trusts their judgment on various sorts of things. Thus if @shlevy wanted to make a change like this to the module system, he might be asking for advice/comments, but not for permission. I realize that this PR (and the others that have died on the vine like it) was phrased a request for comments, but I think there's a fairly widespread sense (perhaps unjustified!) that we shouldn't be making deep changes to things like this without @edolstra's approval. I don't personally know who's credible in what areas of nix, but I'm sure it's easy to figure out, and delegating some of that authority seems like a necessary requisite for this project to scale further, as Eelco has limited time and can't chime in on everything. |
|
The way I read the comments from @shlevy, @aszlig, @copumpkin, @edwtjo, and @wmertens in this thread feels like you all agree that this PR is useful and that it should be merged. At the same time, it's curious that everyone of you has the ability to press the merge button. So why don't you? If you want that patch in Nixpkgs, then merge it! Don't wait for @edolstra to give you permission. His lack of commentary here is a clear indication that he doesn't have an opinion on this matter, so it's pointless to wait for him to make a decision. Clearly he won't. Someone else has to do it. |
|
I'd have pressed that merge button if I fully understood the code, however
|
|
@peti, that boils down to my point earlier:
I think I'm credible in some things and not in others. I like what this PR claims to do, but I don't know if it does it in a good way, so I don't feel like it would be right for me to merge it. I do trust @shlevy's judgment in general, but I also don't know if I'm credible in knowing who to trust, so the same concern applies 😄 Anyway, I'd be happy with a decision structure like the one you're proposing, but I think there should be an explicit statement somewhere (from @edolstra or @rbvermaa) that that's how they want to run the project. I haven't seen such a thing (is there one?), so I default to being fairly cautious, and have observed quite a few other people doing the same. Note that my observation is about caution around deep changes to how nix/nixpkgs/nixos work, not daily packages. I'm not at all paranoid about merging an update to package |
|
I cannot speak for @edolstra or @rbvermaa, obviously, but my personal interpretation of the project's policy is that every "collaborator" is free to commit any change (after some reasonable grace period for others to review it). There's no "ownership" of certain parts of Nixpkgs. If you got commit rights then go ahead and commit! However, if you commit a change that causes trouble for someone else, then the general expectation is that the committer takes responsibility for remedying those issues. This doesn't mean that you have to personally fix every single problem in arcane parts of the package tree, but there should be a sense that the committer takes those issues seriously and makes an effort to get everything back into a working state. If in doubt, that means reverting the entire change and going back to the drawing board. Personally, I won't press the merge button because I don't understand what exactly this patch does and I don't want to be responsible for breaking other people's system in a way that I can neither predict nor fix. My impression is that many others feel the same way. So everyone seems to be looking at @edolstra, because we expect him to understand everything, but that's not a reasonable expectation. My guess is that Eelco has no clue about the potential side-effects of this change either (otherwise he would have pointed them out). |
|
@peti that makes sense to me. Perhaps we should just write down some brief contributor guidelines? Then someone (perhaps even @shlevy, invigorated by newfound enthusiasm stemming from the new guidelines and the ensuing carte blanche) can go revive all these dead PRs. Flowers, rainbows, unicorns, and narwhals will follow 🌷 🌈 🐘 🐳 |
|
Anyway, at this point we are not using these changes and have plans to move from the module system in general, so this particular work is moot for us. |
|
@shlevy "us" there being Zalora, right? The changes might still be relevant to the rest of the Nix community, if someone (presumably not you, then) is willing to put in the work. |
|
Yes, that's what I neant |
|
@shlevy , I will try to review and comment / approve this pull request by the end of next week. |
|
nbp: Thanks a bunch, I'll review/comment it as well as soon as I'm back home =) |
|
I will continue to review these changes later this week. |
|
@nbp I don't really see the use case for this change, which adds a lot of complexity (such as the whole |
|
This does more than just override |
|
The pull-request #6794 is taking over the work made on #2540 , which is included by these changes. This pull-request was only for the the work done in commit 084a017, which sounds like adding some complexity for a small benefit which is yet to be seen. I do not think the module system is changing that frequently to require something which include a specific version of it. I understand the need for using a different of |
|
FWIW, it's not orthogonal, because |
|
Indeed, this is something that I want to address at some point, in order to add logic at the level of the imports such as one module can claim to replace another one, but the |
Building on top of #2540, this allows modules to specify the path of the nixpkgs lib to be used for the evaluation. In addition, if a nix with NixOS/nix@c273c15 is used, they can also specify the scope to use when importing modules, which in conjunction with NixOS/nix@62a6eeb allows the nix path to be set for the module evaluation.