lspce-module: 1.1.0-unstable-2024-07-14 -> 1.1.0-unstable-2024-08-04

[AndersonTorres]

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.

[AndersonTorres]

@jian-lin this is what I was talking about: just promote lspce-module as a regular package and let emacsPackages.lspce concentrate only with "elispy" things.

[jian-lin]

Is this draft ready for review?

[AndersonTorres]

yes

[adisbladis]

Does lspce-module have any use outside of emacs.pkgs? I don't think it should live in by-name?

[jian-lin]

Does lspce-module have any use outside of emacs.pkgs?

I do not think so.

I don't think it should live in by-name?

Agreed.

---

I will add some context.

The motivation of this PR is to make users be able to override lspce.

This PR provides one possible implementation. #329709 is another possible implementation. The code to override lspce of these two methods are very similar.

Note that there is yet another implementation: rewriting lspce with finalAttrs. This method needs #330589.

--- a/pkgs/applications/editors/emacs/elisp-packages/manual-packages/lspce/package.nix
+++ b/pkgs/applications/editors/emacs/elisp-packages/manual-packages/lspce/package.nix
@@ -8,12 +8,9 @@
   yasnippet,
 }:
 
-let
-  lspce-module = callPackage ./module.nix { };
-in
-melpaBuild {
+melpaBuild (finalAttrs: {
   pname = "lspce";
-  inherit (lspce-module) version src meta;
+  inherit (finalAttrs.passthru.lspce-module) version src meta;
 
   packageRequires = [
     f
@@ -22,13 +19,13 @@ melpaBuild {
   ];
 
   # to compile lspce.el, it needs lspce-module.so
-  files = ''(:defaults "${lib.getLib lspce-module}/lib/lspce-module.*")'';
+  files = ''(:defaults "${lib.getLib finalAttrs.passthru.lspce-module}/lib/lspce-module.*")'';
 
   passthru = {
-    inherit lspce-module;
+    lspce-module = callPackage ./module.nix { };
     updateScript = nix-update-script {
       attrPath = "emacsPackages.lspce.lspce-module";
       extraArgs = [ "--version=branch" ];
     };
   };
-}
+})

The code to override lspce is like this:

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

If lspce-module is moved out of passthru, the rewrite looks like:

--- a/pkgs/applications/editors/emacs/elisp-packages/manual-packages/lspce/package.nix
+++ b/pkgs/applications/editors/emacs/elisp-packages/manual-packages/lspce/package.nix
@@ -8,12 +8,9 @@
   yasnippet,
 }:
 
-let
-  lspce-module = callPackage ./module.nix { };
-in
-melpaBuild {
+melpaBuild (finalAttrs: {
   pname = "lspce";
-  inherit (lspce-module) version src meta;
+  inherit (finalAttrs.lspce-module) version src meta;
 
   packageRequires = [
     f
@@ -22,13 +19,14 @@ melpaBuild {
   ];
 
   # to compile lspce.el, it needs lspce-module.so
-  files = ''(:defaults "${lib.getLib lspce-module}/lib/lspce-module.*")'';
+  files = ''(:defaults "${lib.getLib finalAttrs.lspce-module}/lib/lspce-module.*")'';
+
+  lspce-module = callPackage ./module.nix { };
 
   passthru = {
-    inherit lspce-module;
     updateScript = nix-update-script {
       attrPath = "emacsPackages.lspce.lspce-module";
       extraArgs = [ "--version=branch" ];
     };
   };
-}
+})

And the override code is simpler:

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

[AndersonTorres]

Migrated to emacsPackages.

[AndersonTorres]

Anything else, @jian-lin @adisbladis ?

[jian-lin]

A similar question: does lspce-module have any use outside of emacs.pkgs.lspce? I do not think it should live in emacs.pkgs. I may change my mind if you give me some examples in other package sets.

[AndersonTorres]

In practical terms, it does not. The only package that really needs lspce-module is lspce-elisp itself.

On the other hand:

• it is a good thing to override lspce-module
  • because it should not be a mere implementation detail - a postulate of this PR
• for all intents and purposes, lspce-module is a package
• overridable packages (if not all packages) are usually reachable from top-level
  • whether if directly (lspce) or indirectly (emacs.pkgs.lspce)
  • by far this the easiest way of overriding them, at least for now

"Not being useful outside lspce-elisp" does not look like a sufficient motivation to avoid the detachment.

(Unfortunately I can't think about packages with the sole purpose of leveraging other single package; however I don' t believe they don't exist. Bootstrap seeds are a perfect example of this.)

[jian-lin]

"Not being useful outside lspce-elisp" does not look like a sufficient motivation to avoid the detachment.

I am not convinced and I still think top-level packages should be useful on it own.

* it is a good thing to override `lspce-module`
  
  * because it should not be a mere implementation detail - a postulate of this PR

Why should it not be a mere implementation detail? lspce-module being only useful inside lspce indicates that lspce-module is an implementation detail.

Even as an implementation detail, i.e., non top-level, it can still be overridden. See #329709 (comment)

* for all intents and purposes, `lspce-module` is a package

Being a package does not mean being a top-level package.

I think lspce-module being a package is caused by limitation of current build helpers. See bellow for more info. Ideally, it should not be a package and the module written in Rust can be built inside the melpaBuild of lspce just like how emacs.pkgs.rime builds its module (written in C) inside melpaBuild.

* overridable packages (if not all packages) are usually reachable from top-level
  * by far this the easiest way of overriding them

With this PR applied, code to override lspce looks like

final: prev: {
  lspce-module = prev.lspce-module.overrideAttrs { ... };
  lspce = prev.lspce.overrideAttrs {
    inherit (final.lspce-module) version src;
  };
}

An alternative PR #329709 does not treat lspce-module as a top-level package. With that PR applied, code to override lspce looks like

final: prev: {
  lspce = let lspce-module' = prev.lspce.lspce-module.overrideAttrs { ... }; in
    (prev.lspce.override { lspce-module = lspce-module'; }).overrideAttrs (old: {
    inherit (lspce-module') version src;
    passthru = old.passthru // { lspce-module = lspce-module'; }; # optional
  });
}

To be honest, I do not think one code is easier than another. Both of them are not good because users have to know that lspce depends on lspce-module and have to change two places to achieve the goal of overriding one package.

I think the real issue is that currently different build helpers (rustPlatform.buildRustPackage and emacs.pkgs.melpaBuild here) does not compose, meaning you cannot call stdenv.mkDerivation only once when building a package using more than one language. Well, you can if your two languages are C/C++ and something else. But that is because stdenv.mkDerivation supports C/C++.

(Unfortunately I can't think about packages with the sole purpose of leveraging other single package)

I do find an example supporting my opinion: vim-clap.

[AndersonTorres]

I was elaborating an answer, however it would not be satisfactory.

My objection with your PR is, basically, two things:

• with one hand your are hiding a thing (lspce-module is not reachable from top-level);
• with the other hand you are exposing the same thing (lspce-module is a parameter).

---

The main problem is what you said, the builders are not composable.
Indeed my impression is that it was never cogitated before.

The examples you provided are not convincing, I should say.

• vim-clap does not allow overriding maple. Indeed vim-clap is equivalent to the current status quo of lspce-module.
• emacs.pkgs.rime is not exactly built inside melpaBuild. It is overriden in the file melpa-packages.nix:

        rime = super.rime.overrideAttrs (old: {
          buildInputs = (old.buildInputs or [ ]) ++ [ pkgs.librime ];
          preBuild = (old.preBuild or "") +
          (if pkgs.stdenv.isDarwin then
            ''
              export MODULE_FILE_SUFFIX=".dylib"
              make lib
              mkdir -p /tmp/build/rime-lib
              cp *.dylib /tmp/build/rime-lib
            ''
          else
          ''
            make lib
            mkdir -p /build/rime-lib
            cp *.so /build/rime-lib
          '');
          postInstall = (old.postInstall or "") +
          (if pkgs.stdenv.isDarwin then
          ''
            install -m444 -t $out/share/emacs/site-lisp/elpa/rime-* /tmp/build/rime-lib/*.dylib
          ''
          else
          ''
            install -m444 -t $out/share/emacs/site-lisp/elpa/rime-* /build/rime-lib/*.so
          '');
        });

How should rime be built if it was not accepted by MELPA?

The best I can think in this situation is to inject lspce-module as an extra attribute inside melpaBuild. I will upload an extra commit.

[jian-lin]

• emacs.pkgs.rime is not exactly built inside melpaBuild. It is overriden in the file melpa-packages.nix

Why is overrideAttrs relevant here? The point is that only one build helper (melpaBuild) is used so there is no composability issue.

How should rime be built if it was not accepted by MELPA?

Create a normal manual elisp package and move what is now in overrideAttrs to melpaBuild. That's it. Nothing special.

Example

{
  melpaBuild,
  fetchFromGitHub,
  librime,
  dash,
  popup,
  posframe,
}:

melpaBuild {
  pname = "rime";
  version = "1.0.5-unstable-2024-01-30";

  src = fetchFromGitHub {
    owner = "DogLooksGood";
    repo = "emacs-rime";
    rev = "d8c0a99b0282d3e0aca53146789f6864181228e7";
    hash = "sha256-rx9SvIuMVL0Vt1uKbLhxbxTKyftdRp/BVj71hJIyQi4=";
  };

  packageRequires = [
    dash
    popup
    posframe
  ];

  buildInputs = [ librime ];

  preBuild = ''
    make lib
    export RIME_LIB_DIR=$NIX_BUILD_TOP/rime-lib
    mkdir -p $RIME_LIB_DIR
    cp *.so $RIME_LIB_DIR
  '';

  postInstall = ''
    install -m444 -t $out/share/emacs/site-lisp/elpa/rime-* $RIME_LIB_DIR/*.so
  '';

  ignoreCompilationError = false;
}

The best I can think in this situation is to inject lspce-module as an extra attribute inside melpaBuild.

What is the point of this completely syntactic refactoring? I fail to see how it improves user experience for overriding. In this implementation, to override lspce, files still has to be changed besides src, version and lspce-module.

---

About composability, I find that with some modifications, the rust build helper does compose well with elisp build helpers. See the rewriting of lspce in #334476.

[AndersonTorres]

What is the point of this completely syntactic refactoring? I fail to see how it improves user experience for overriding.

It would be visible to overrideAttrs instead of override.