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.

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

implement builtinPackagesBuilder #28

Merged
merged 10 commits into from
Apr 16, 2021
Merged

Conversation

blaggacao
Copy link
Collaborator

@blaggacao blaggacao commented Apr 12, 2021

depends on numtide/flake-utils#28
closes #12

This is a convenient function that enables sane default sharing of channel overlays:

  • It filters on packages per channel, that are also defined in overlays
  • therefore, only "custom" stuff that adds potential value to the world is exported
{
  self.packages."<system>" = {
    # only packages per system who's checks do not trivially fail:
    # - system compatible
    # - a derivation 
    # - meta.broken == false
    # and only packages that are defined by:
    # - some overlay 
    # - on some channel.
    # that includes classical override variants of packages, 
    # as well as new package definitions.
    kakoune = { ... };
  };
}

checpoint w/o the following:

  • cleanup / refactoring
  • docstrings
  • tackle nixpkgs.lib-is-not-cheap-to-import-issue
  • discuss packageBuilder api extension with channels

@blaggacao blaggacao force-pushed the da/default-packages-builder branch 3 times, most recently from 8bacf7a to 7c642f3 Compare April 12, 2021 04:01
@gytis-ivaskevicius
Copy link
Owner

I won't pretend that I understand what you are doing here - May I see some actual examples/use cases?

Currently, overlays are applied to channels and packages/system configurations/repl are being populated from there - and I do quite like this approach, data flow is nice and simple.

As for exposing overlays - it is done by standard overlay/overlays attributes, just like in normal flakes. https://github.com/gytis-ivaskevicius/nixfiles/blob/master/flake.nix#L148

@blaggacao
Copy link
Collaborator Author

blaggacao commented Apr 13, 2021

May I see some actual examples/use cases?

I definitly owe better context, still. I'll give a peak, here:

The problem this PR tries to solve goes by this storyline:

What is useful content of self.packages? self.packages is a way to share one's art with others so for example, if I'd packages latest FRRouting, which is not available on Nixpkgs, then people might have an interest in using that.

One mediumly sane assumption could be that everything for which I built an overlay might also be worth sharing with the world.

This PR builds on that assumption and provides a mean to export all packages that where declared through any of the channel overlays.

I still need to do the other half of the work on this like cleanup and some refactoring. If you are wary to accept such proposal, I can very well understand you and no worries, I'd include it in devos, instead.

@gytis-ivaskevicius
Copy link
Owner

gytis-ivaskevicius commented Apr 13, 2021

Alright, I hear you, but let me raise this point:
Is self.packages actually a good way of sharing packages? Nix provides 2 types of packages of sorts - overlays and actual packages, the difference is that the latter one depends on the maintenance of the existing repository.

Sharing using overlays may not be entirely straight forward but in better solution due to these reasons:

  1. Dependency (inputs) reusability
  2. Dependency (inputs) updates

The downside is that it involves a few extra steps:

  1. Apply overlay to your main channel
  2. Install applied package (which name you might need to manually lookup)
  3. If it works - all good, if it does not - apply the overlay to an unstable channel and install it from there

Upsides of self.packages:

  1. One can do stuff like nix run github:gytis-ivaskevicius/nixfiles#g-neovim

I strongly dislike the concept of packages, and honestly, it makes me wonder if it would be possible for nix to have only overlays with the addition of "strong" and "weak" dependency concept where weak dependencies would be replaced depending on the user-specified channel and strong dependencies would be explicitly specified in the package maintainer flake

Anyways, I don't mind this feature, but implementation and especially the user-facing side must be pretty sleek

@blaggacao
Copy link
Collaborator Author

blaggacao commented Apr 13, 2021

Ha, you have put my nacent thought in even better words, here.

For the "strong" & "weak" dependency idea:

In order to give consumer at least a visual clue about what channel aproximately an overlay was made for, we planned to export overlays primarily as:

{
  self.overlays = {
    "unstable/kakoune" = final: prev: { };
    "nixos-20.09/firefox" = final: prev: { };
  };
}

I say "aproximately", because "unstable" not necesarily means the same thing in terms of compatibilty across repositories.

@gytis-ivaskevicius
Copy link
Owner

gytis-ivaskevicius commented Apr 13, 2021

I had in mind a much more fine graned idea of weak/strong dependencies, way too complex and out of scope for fup and devos. But yeah.. It does not really matter

Also, I doubt that overlay exporting like that is necessary especially since there is a big difference between unstable 1 day ago vs unstable 6months ago.

Anyways, we will see :)

@blaggacao
Copy link
Collaborator Author

blaggacao commented Apr 13, 2021

Maybe you are right and we should discourage the use of self.packages in the context of a nixos sharing model.

As I wrote my peak, this possibility was already dawning in my head.

It is still a good tool for software projects, but it is arguably less useful for the nixos sharing model.

@blaggacao
Copy link
Collaborator Author

You are also right about unstabld, a better semantic might be, if we can source that distincion meaningfully from the inputs...

{
  ​self.overlays​ ​=​ {
    ​"​kakoune​"​ ​=​ ​final​: ​prev​: { };
    ​"​nixos-20.09/firefox​"​ ​=​ ​final​: ​prev​: { };
  };
}

@Pacman99
Copy link
Collaborator

There is one big advantage of packages, and I don't think any of these messages touched it. Packages are entirely self contained. Overlays depend on a specific version of nixpkgs, because they build the package using the version of nixpkgs given to them(prev: final:). This point was raised already.

Whereas when I get a package from a flake, it has already been created with some version of nixpkgs. And while this might result in dependency reuse, I am guaranteed that building that package will work, because of nix. So in my opinion overlays are more convenient, but packages are a much better way of sharing your packages because they provide safety riding on nix's guarantees.

This is sort of like if I had tested firefox on nixpkgs/someCommit and told you that it worked. If you install firefox from that same nixpkgs commit, it WILL work for you. In the same vein I am creating my package with a version of nixpkgs that I know works, so therefore I can share it with much more safety. This is not true for overlays, because you are using an "untested" version of nixpkgs to build my package. And even if you prefer using overlays to share packages, we should still consider the advantages of sharing via packages.

systemFlake.nix Outdated Show resolved Hide resolved
@blaggacao
Copy link
Collaborator Author

I suspect, a good argument to consider for the sharing model, though are distributed binary caches.

They would very likely only work on unmodified self.packages and wouldn't probably work on overlays.

Sure this goes at the expens of tons of GB in data, but in some low-optimized environments, this might simply afford us the better user experiemce.

(Just thinking loud)

@blaggacao blaggacao force-pushed the da/default-packages-builder branch 2 times, most recently from 34d3fdf to 286c625 Compare April 13, 2021 22:25
Copy link
Collaborator

@Pacman99 Pacman99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this! It has a lot of potential to improve user's sharing model.

systemFlake.nix Outdated Show resolved Hide resolved
overlaysPackagesBuilder.nix Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes to self

flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
@blaggacao blaggacao force-pushed the da/default-packages-builder branch 3 times, most recently from d11df81 to 8776e0b Compare April 14, 2021 15:59
@blaggacao blaggacao marked this pull request as ready for review April 14, 2021 15:59
@blaggacao blaggacao force-pushed the da/default-packages-builder branch 2 times, most recently from 3d097c9 to 33bcda7 Compare April 14, 2021 16:27
Comment on lines +17 to +19
moduleA = import ./path/to/moduleA.nix;
moduleBfolder = import ./path/to/moduleBfolder;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While working on the docs for moduleFromListExporter, I was wondering if we would take good advice here and overload those with some metadata concerning either or a combination of:

  • location
  • ownership (by this flake's current world-wide unique commit hash)

Prospectively, those enable us to distinguish in a global namespace which modules to auto-export by default without the user in need to tell us.

@Pacman99 How could such metadata enrichment potentially look like at this position?

I'd very much favor to record ownership by a specific commit hash, here, since it seems universally useful and directly testable by a downstream user's flake's self.rev.

Copy link
Collaborator Author

@blaggacao blaggacao Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
  modulesFromListExporter = paths: owningRev: { };
}

(if that makes sense)


maybe also

{
  overlaysFromChannelsExporter = channels: owningRev: { };
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use __functor for this. The overlay/module can go in __functor and metadata can be part of the attrset. I would say save this for future work however.

@blaggacao
Copy link
Collaborator Author

@gytis-ivaskevicius did you already make your mind up on the general direction that this proposal is taking? In a sort is perpetuates the direction the modulesFromList previously has set and I believe it is a plausible (intermeadiate) answer to the sharing model we could encourage.

Intermediate, since we might well need to iterate on the details after collecting some more experience or as flakes themselves evolve.

Copy link
Owner

@gytis-ivaskevicius gytis-ivaskevicius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be very pleased with having all this as part of fup 👍

Few general comments:

  • inconsistent use of inherit (builtins) mapAttrs; and builtins.mapAttrs
  • very odd spacing
  • some let/in statements and function calls don't make much sense - let/in syntax should be removed in a couple of locations

Also, another random idea: something along the lines of $ nix run build development could be implemented so users would have easier time building their aplications using Github actions

overlaysFromChannelsExporter.nix Outdated Show resolved Hide resolved
Comment on lines 36 to 38
overlays' = [
"development"
];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cool to make this typesafe, something along the lines of:

    overlays' = [
      xyz.development
    ];

Just some food for thought, dont mind me :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is in fact stringly typed on purpose for the (string) prefix matching to work after pkgs' tree has been flattened. I also wouldnt be able to tell what xyz could be, since overlays define top level attributes on the pkgs namespace.

systemFlake.nix Outdated Show resolved Hide resolved
@blaggacao
Copy link
Collaborator Author

I think I mostly addressed your concerns. I think nix run build development is +- enabled through packagesFromOverlaysBuilderConstructor, It's just that each package has to be called individually. I might assume your idea is to have the convenience of just having one single "build job". It would be actually interesting to explore how that could best work with the jobs or hydraJobs outputs (job for the sake of being agnostic).

@gytis-ivaskevicius
Copy link
Owner

This code does not even parse, what's up with that? How does one even work on a feature when that is the case?

@blaggacao
Copy link
Collaborator Author

I got tossed away from keyboard last night. I should have put that back into draft, strictly speaking. 😉

David Arnold added 8 commits April 16, 2021 11:40
This prepares for adding additional exporters and builders in following
commits.

The implementation of modulesFromList is no changed in any way.
overlaysFromChanelsExporter generates a flat namsepaced overlay
attribute set that exports every overlay key from every channel
individually.

Good for users that want to pull in individual overlays and evaluate
them with their own dependencies.
Constructor, that with the help of channels dependency constructs a
packagesFromOverlaysBuilder that outputs all packages defined
in overlays that do not break flake checks.

Good for sharing binary cached packages. Any package that would
break nix flake check (e.g. because it is marked as broken) cannot
be shared via this mechanism, and the alternative via overlays has to be
used.
@blaggacao
Copy link
Collaborator Author

I'm going to merge this, since it's the last piece missing for us to rebase devos for which we have a +- plan to finalize it by this weekend. It also does not really hurt any existing workflow since it mostly adds features, and I don't know how to test it really properly (other than rebasing devos which will make intricate use of it).

To address the latter I opened #35, which should help with that, going forward.

@blaggacao blaggacao merged commit 141f448 into staging Apr 16, 2021
@blaggacao blaggacao deleted the da/default-packages-builder branch April 25, 2021 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants