Skip to content
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.

Sign up for GitHub

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

Jump to bottom

WIP lib: Better extensible attrsets #51213

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

WIP lib: Better extensible attrsets #51213

wants to merge 9 commits into from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Nov 29, 2018 •
edited

Motivation for this change

This is a WIP implementation of something like @edolstra's {{...}} recursive attrsets (from older https://gist.github.com/edolstra/29ce9d8ea399b703a7023073b0dbc00d), but with the self-super pattern. I'm all for the more ambitious goals (sharing abstractions for nixos/nixpkgs, no callPackage, prioritizes or something finer-grained over super sledge-hammer, etc), but fear without incremental steps like this, they won't happen.

  • Like it or not, self super is today's semantics for Nixpkgs
  • Doesn't try to fight callPackage, works very well with self.callPackage in the manner of Haskell package sets.
  • Recursion on merge solves the problem of deep overrides in overlays / recursive package sets

In particular, having both the inner and outer fixed points solves the problems with #44196 and my injectOverride alternative:

  • "Copying" (rebinding) an inner extensible attribute set just works, future overrides of the original won't affect the copy
  • overriding inside works per recursive merge
  • no overrideScope needed as one can just use makeScope to bind a local callPackage which will be updated automatically by extensible attrset merge.

The last commit is an example rushed example of how it simplifies llvmPackages_5 and darwin inner attribute set overriding in the Darwin bootstrapping. Further simplification would come from the unrelated change of having just llvmPackages_*.{tools,libraries}, rather than that + the legacy smushing of everything inside those alongside.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

CC @ElvishJerricco

Ericson2314 and others added 9 commits November 28, 2018 23:12
@Ericson2314
lib: Add notion of a monoid. WIP need tests!
ef718fe
@Ericson2314
lib: Add monoidal fold
e48d89a
@Ericson2314
lib: Remove makeExtensibleWithCustomName
2debf56 
There are no more custom names that need this. Normal `makeExtensible`
is fine.

This reverts commit e4cd45a.
@Ericson2314
lib: Add bootstrapping stage
5eeeb47 
The definition of `makeExtensible` is about to become more involved,
such that we cannot just get it out of one file. As such, we first need
to construct a non-overridable version of the library to get it, and
then construct the final overridable version of the library.
@Ericson2314
lib: Create notion of extensibleAttrset
4b3b945 
These automatically compose in overlays, giving us the best of both
worlds between nested fixed points and a single outer fixed point.
@Ericson2314
lib: Use makeExtensibleAttrset in makeScope
14bfa6d
@Ericson2314
stage.nix: turn all overlays into extensible attrsets and combine tha…
48b20f3 
…t way
@Ericson2314
llvmPackages_*: Make sure all the way up to the top is an extensibleA…
5e5ffaf 
…ttrset
@Ericson2314
darwin stdenv: Demo new extensible attrsets
a09ba44
@GrahamcOfBorg GrahamcOfBorg added the 6.topic: stdenv Standard environment label Nov 29, 2018
@edolstra
Copy link
Member

No, this is a wrong direction for Nix. I don't want to have to explain to users what a "monoidal extension" is. The Nix language (as a DSL) should provide the proper concepts, they should not be provided in a library. Also, I suspect that this will make Nixpkgs evaluation even slower and memory-hungry.

@Ericson2314
Copy link
Member Author

@edolstra don't miss the forest for the trees :)

  1. All the monoid stuff is just an implemention detail. It can be inlined away.

  2. If we make zipAttrsWith a built-in there should be little performance impact. (Typically the overlay atteset is vastly smaller, and the overriding vs new attrs vastly smaller than that.)

@Ericson2314 Ericson2314 mentioned this pull request Dec 5, 2018
Closed
@Ericson2314
Copy link
Member Author

Ericson2314 commented Dec 5, 2018 •
edited

Also, there's a separate issue that it makes little sense to implement a DSL and try it out in one step: One cannot know a priori if the abstraction works, and changing C++ Nix everytime the abstraction needs tweaking is too slow a debug cycle.

Changes like this raise the bar, slowly kneading nixpkgs into shape towards the final DSL. Both the refactoring of existing packages and fleshing out of the DSL can take place incrementally. Only once we've converged on a stable interface that's used by a wide range of package does it make sense to freeze and enshrine it with new syntax.

@ElvishJerricco
Copy link
Contributor

I agree with @Ericson2314 about trying to do things incrementally; we're not going to be able to convert all of nixpkgs to the new system at once whenever we do get the language level feature. But @Ericson2314, I would recommend finding more friendly names for this stuff. IMO this has little to do with the term "monoid", because it's not actually abstract over the monoid in use. It'd be like calling || "monoidal choice".

@edolstra
Copy link
Member

edolstra commented Dec 5, 2018

@Ericson2314 That's what we've been doing for the last few years: we have stuff like the module system as a library feature. Now it's time to do it properly as a language feature.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Dec 6, 2018 •
edited

@ElvishJerricco no it actually is abstract over all monoids in this implementation, just as we talked about long ago. The user doesn't need to worry about arbitrary monoids, but by the same token it is bad and a sign of the WIP-ness of this PR that they currently use any of the monad-polymorphic stuff.

@edolstra Yes, NixOS is ready, but Nixpkgs-the-package-set isn't. The most widely used things in nixpkgs like stdenv and cc-wrapepr are known-bad, and the langauge-specific/nested package stuff is not at all standardized. If your proposal only talked about NixOS, it would be one thing, but as it aims for the unified abstraction for packages and configurations, we have no choice but to first force Nixpkgs-the-package-set into good condition.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Dec 6, 2018 •
edited

@edolstra @ElvishJerricco and @angerman in particular are doing some cool work with next-gen Haskell infra using the NixOS module system. A wonderful yet realistic sequence of events to me looks like:

  1. Merge cleaned up version of this
  2. Get everything in nixpkgs using nested self: super: { .. } package sets / overriding
  3. New Haskell stuff done with NixOS modules, possible extended modules
  4. Use @Profpatsch's type checker work to separate merging / checking aspects of module system
  5. Use Module system instead of self: super: { .. } package sets.
  6. Deal with any remaining callPackage and friends (e.g. make that compat layer) (Your older stuff / @nbp SOS).
  7. NixOS + Nixpkgs all uses improved module system (maybe with some deprecated layers)
  8. Some static typing stuff is finally ready?
  9. Enshrine everything with new syntax.

@Ericson2314 Ericson2314 mentioned this pull request Jan 18, 2019
Closed
@lopsided98 lopsided98 mentioned this pull request Jan 18, 2019
Closed
9 tasks
@mmahut
Copy link
Member

mmahut commented Aug 25, 2019

Are there any updates on this pull request, please?

@FRidh FRidh mentioned this pull request Sep 17, 2019
Closed
@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@infinisil
Copy link
Member

Relevant rfc: NixOS/rfcs#67

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@ryantm ryantm marked this pull request as draft October 23, 2020 03:10
@stale
Copy link

stale bot commented Apr 26, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@nbp nbp Awaiting requested review from nbp

@edolstra edolstra Awaiting requested review from edolstra

@infinisil infinisil Awaiting requested review from infinisil infinisil will be requested when the pull request is marked ready for review infinisil is a code owner

Assignees
No one assigned
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: work-in-progress 6.topic: stdenv Standard environment
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Ericson2314 @edolstra @ElvishJerricco @mmahut @infinisil @ryantm @GrahamcOfBorg