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

Make overrides an overlay type rather than a functionTo #67

Merged
merged 8 commits into from
Feb 10, 2023

Conversation

srid
Copy link
Owner

@srid srid commented Feb 7, 2023

A step towards #12

  • proof of concept really decouple the nixpkgs workaround haskell-template#85
  • new overlay option type
    • make it mergeable
    • finish mkOption implementation (loc, etc.)
  • test mergeability of other options (devShell.tools, source-overrides, etc.)
  • how to publish these modules?1
  • docs & changelog
  • proper tests

Footnotes

  1. From @roberth on Matrix:

    I think what's missing from the picture is a flake attribute for publishing those modules, and setting up the module arguments for those modules so that they don't have to rely on variables from the lexical scope. The lexical scope is too specific to the local project, not including things like the consuming project's pkgs and whatnot [..] oh and for publishing a module, you can use types.deferredModule, which is takes modules, turns them into a single module, but does cause them to be evaluated; returning a module syntax instead of an evaluated configuration

flake-module.nix Outdated
Comment on lines 74 to 75
submoduleWithImports = mod: types.submoduleWith { modules = lib.toList mod; };
projectSubmodule = submoduleWithImports (args@{ name, config, lib, ... }: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Done in #68

flake-module.nix Outdated
descriptionClass = "noun";
check = lib.isFunction;
merge = loc: defs:
# TODO: What to do with loc?
Copy link
Collaborator

Choose a reason for hiding this comment

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

loc is for reporting the definition locations when doing type checking.
I don't think there's a function we could reuse for this, but Nixpkgs lib/types.nix has "example" code to work from.
The reason we can't reuse anything is because the interaction between type checking and laziness is quite subtle. The module system hasn't elaborated all the abstractions that might make this stuff obvious, and I don't know if that can be done at all and without making it slow and confusing.

In this case, I don't think we should use composeManyExtensions, because we can't really check the function bodies before know what the arguments are.
Another possible issue is that the ordering of the overlays matters, but is quite arbitrary in the module system. Ideally, modules are structured in such a way that the order in which the modules are processed does not matter, but for list and some overlays it does. We might want to detect that; see NixOS/nixpkgs#215486

So this type is a bit of a rabbit hole. We'll want to go for incremental improvements. No checking at all is a valid first solution.

Copy link
Owner Author

@srid srid Feb 9, 2023

Choose a reason for hiding this comment

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

In this case, I don't think we should use composeManyExtensions, because we can't really check the function bodies before know what the arguments are.

But this is not important, right? The error message may not be helpful, but an error is thrown nonetheless, forcing the user to specify the overlay correctly.

Another possible issue is that the ordering of the overlays matters

Is this a blocker for implementing composable overlays that can be import'ed, then? Or, is the order not deterministic for simpler cases. Like take this one,

{ 
  haskellProjects.foo = {
    imports = [ inputs.myCompanyCommon.haskellFlakeProjectDeps ];
    overrides = self: super:  { /* Local project overrides go here */ };
  };
}

Here, I would imagine the global dependency overrides from myCompanyCommon repo to apply first before the project overrides are applied, right?

If the order is deterministic, I was thinking we could note the limitation of arbitrary order in the option docs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The ordering of the definitions passed to the merge function is deterministic in the sense that it only depends on the modules that are loaded and the implementation of the modules system, but I can't tell you what the order would be. It depend on details about how the import closure is computed and when and how often the module list is reversed. Also I don't want to know because it's a better idea for scalability to avoid depending on this implementation detail. If you move an override to another module, and the code breaks, wouldn't that be really really bad? It'd be unreliable and it stops you from refactoring the code, so that will become a mess.

I think we can just document it for now.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Let's add it as an experimental feature and re-evaluate in future after it seeing some practical uses. The use of >1 overlays will also result in this warning to the user:

trace: WARNING[haskell-flake]: Multiple haskell overlays are applied in arbitrary order.

Copy link
Owner Author

Choose a reason for hiding this comment

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

By the way, even without this PR this problem remains - attrset merging (which functionTo relies on) too uses arbitrary order, but there it is not even using the overlay composition.

@srid srid force-pushed the project-imports-simple branch from e31967b to efc8b4d Compare February 10, 2023 16:29
@srid srid changed the title WIP: import'able project settings Make overrides an overlay time rather than functionTo Feb 10, 2023
@srid srid changed the title Make overrides an overlay time rather than functionTo Make overrides an overlay type rather than a functionTo Feb 10, 2023
@srid srid marked this pull request as ready for review February 10, 2023 20:05
@srid srid merged commit 4585d42 into master Feb 10, 2023
@srid srid deleted the project-imports-simple branch February 10, 2023 20:07
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.

2 participants