Skip to content
This repository was archived by the owner on Mar 11, 2024. It is now read-only.

use-case: deeply replace packages#12

Merged
alex-ameen merged 17 commits intomainfrom
deep-replace
Jun 25, 2023
Merged

use-case: deeply replace packages#12
alex-ameen merged 17 commits intomainfrom
deep-replace

Conversation

@alex-ameen
Copy link
Contributor

No description provided.

@roberth
Copy link

roberth commented May 9, 2023

Can I pass the baton to @nbp?

EDIT: looks decent, but I don't know the review criteria as I'm not directly involved with the working group.

@roberth roberth removed their request for review May 9, 2023 15:27
@alex-ameen alex-ameen requested a review from infinisil May 12, 2023 13:51
@alex-ameen alex-ameen marked this pull request as draft May 21, 2023 15:23
Copy link
Contributor

@phaer phaer left a comment

Choose a reason for hiding this comment

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

LGTM, except for the pending update of the example :)

alex-ameen and others added 2 commits May 26, 2023 09:36
Co-authored-by: Silvan Mosberger <silvan.mosberger@tweag.io>
Copy link

@KFearsoff KFearsoff left a comment

Choose a reason for hiding this comment

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

LGTM, but I think a little more detail should be given about the "deep replacing" exactly.

I think providing a convenient way to deep-replace is cool, but I think an even better solution would be making deep-replace the baseline, and shallow replace to be opt-in.

Perhaps an example would help illustrate the point. I imagine an API like this:

  • Deep replace (default and easy):
let patched-treesitter = pkgs.nvim-treesitter { # pkgs.nvim-treesitter is a module in this context
  src = { # submodule that is merged on top of defaults
    version = "1.2.4"; # instead of 1.2.3 in this example
    sha256 = "<sha256>";
  };
};
nvim-treesitter.enable = true;
nvim-treesitter.package = patched-treesitter;
nvim-treesitter-terraform.enable = true; # will use patched treesitter
  • Shallow replace:
let patched-treesitter = pkgs.nvim-treesitter { # pkgs.nvim-treesitter is a module in this context
  src = { # submodule that is merged on top of defaults
    version = "1.2.4"; # instead of 1.2.3 in this example
    sha256 = "<sha256>";
  };
};
nvim-treesitter.enable = true;
nvim-treesitter-terraform.enable = true;
nvim-treesitter-terraform.package.inputs = { # attrset of modules
  nvim-treesitter = treesitter-patched; # replace input module with our module
};

The main challenge here would be that modules are mutable, and we need to create a deep clone of a module to customize it. We also have to be wary of infinite recursion. But I do feel that "Deep replace" should be the expected behavior when you override hello.package option, and "Shallow replace" should be the expected behavior when you override hello.inputs.glibc.

EDIT: I think that API would be quite possible if a "package module" is a function like this: overrideArgs ? {}: if (overrideArgs == {}) then (module) else (overridenModule)

@dblsaiko
Copy link

dblsaiko commented Jun 9, 2023

security patching

This would cause full rebuilds unlike NixOS/nixpkgs#10851 wants to do however, from what I can see, right?

alex-ameen and others added 2 commits June 13, 2023 10:03
Co-authored-by: Benoit de Chezelles <bew@users.noreply.github.com>
@alex-ameen alex-ameen requested a review from infinisil June 13, 2023 15:04
@alex-ameen alex-ameen marked this pull request as ready for review June 23, 2023 13:11
@alex-ameen alex-ameen requested review from KFearsoff, bew and phaer June 23, 2023 13:14
Copy link
Contributor

@phaer phaer left a comment

Choose a reason for hiding this comment

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

LGTM!

Co-authored-by: Paul Haerle <hello@phaer.org>
@alex-ameen alex-ameen merged commit ab4677f into main Jun 25, 2023
@DavHau
Copy link
Contributor

DavHau commented Jul 7, 2023

@aakropotkin In an earlier meeting (which you might have missed) we agreed that the approval of all individual team members is required before a PR can get merged. Therefore this PR has been merged too early.

@phaer phaer deleted the deep-replace branch July 8, 2023 05:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document use case: Deeply replacing packages in Nixpkgs, e.g. security patching

8 participants