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

emacsPackages.lspce: make users be able to override it #329709

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jian-lin
Copy link
Contributor

Without this patch, users cannot bump lspce locally because they cannot override it. With this patch applied, they can use the following code snippet to do so.

let
  epkgs = pkgs.emacs.pkgs;
  lspce-module = epkgs.lspce.lspce-module.overrideAttrs (old: { ... });
in
(epkgs.lspce.override { inherit lspce-module; }).overrideAttrs (old: {
  inherit (lspce-module) version src;
  # optional
  passthru = old.passthru // {
    inherit lspce-module;
  };
})

This patch does not change the outPath of lspce.

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Without this patch, users cannot bump lspce locally because they
cannot override it.  With this patch applied, they can use the
following code snippet to do so.

```nix
let
  epkgs = pkgs.emacs.pkgs;
  lspce-module = epkgs.lspce.lspce-module.overrideAttrs (old: { ... });
in
(epkgs.lspce.override { inherit lspce-module; }).overrideAttrs (old: {
  inherit (lspce-module) version src;
  # optional
  passthru = old.passthru // {
    inherit lspce-module;
  };
})
```

This patch does not change the outPath of lspce.
@jian-lin jian-lin requested a review from adisbladis as a code owner July 24, 2024 18:02
@github-actions github-actions bot added the 6.topic: emacs Text editor label Jul 24, 2024
@AndersonTorres
Copy link
Member

AndersonTorres commented Jul 24, 2024

Well, it looks like a convoluted version of what I did before, namely, promoting lspce-module to a top-level package.

Further, given what happened at #324199 and #325517, it is better to top-level it.

(TL;DR - the parameter was overridden by a custom overlay.)

@jian-lin
Copy link
Contributor Author

given what happened at #324199 and #325517, it is better to top-level it

(TL;DR - the parameter was overridden by a custom overlay.)

After skimming through most of the context I find from links of the PRs you provide, I think that issue is not relevant here. The issue mentioned in those PRs is that default values of arguments can be changed by callPackage. However, in this PR, the functionality of callPackages to fill in arguments is not used because we explicitly pass the argument: lspce = callPackage ./manual-packages/lspce { lspce-module = callPackage ./manual-packages/lspce/module.nix { }; }. So even if some user has an elisp package called lspce-module, it does not break anything.

By "top-level it", I assume you mean moving lspce-module into pkgs and changing lspce = callPackage ./manual-packages/lspce { lspce-module = callPackage ./manual-packages/lspce/module.nix { }; } to lspce = callPackage ./manual-packages/lspce { inherit (pkgs) lspce-module; }. Then I think topleveling it is worse than this PR because pkgs.lspce-module can still be changed by an overlay. Note that, this PR does not have this problem.

it looks like a convoluted version of what I did before, namely, promoting lspce-module to a top-level package.

I do not think this PR is like topleveling lspce-module.

However, I do agree that override ing it is a bit convoluted. I think making melpaBuild (and elpaBuild and trivialBuild ) understand the finalAttrs pattern can help. I think it is doable and will create a PR later.

After that, we can override lspce like this. Does this look better to you?

pkgs.emacs.pkgs.lspce.overrideAttrs (old: {
  lspce-module = old.lspce-module.overrideAttrs (old': {
    version = ...;
    src = ...;
  });
})

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jul 24, 2024
@AndersonTorres
Copy link
Member

AndersonTorres commented Jul 25, 2024

Well, thinking a little bit more, the override that I cited as a bug in the links above is indeed an expected outcome here.

E.g.: if I create a custom overlay that overrides GCC, say gcc-inject-fences, it is expected that this GCC will affect stdenv and the whole tree as a consequence.

Nonetheless, I think this code is similar in purpose to my old proposal: the lspce-elisp code now has lspce-module as an explicit input.
So, why not to expose lspce-module as a legitimate package - as an expression inside pkgs/by-name/lspce-module/package.nix?

Since the idea is to allow the override of lspce-module, exposing it makes it even more discoverable.


I think making melpaBuild (and elpaBuild and trivialBuild ) understand the finalAttrs pattern can help. I think it is doable and will create a PR later.

Regardless what I said before, this will be nice!

@jian-lin jian-lin marked this pull request as draft July 26, 2024 03:20
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: emacs Text editor 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants