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.

Sign up for GitHub

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

Jump to bottom

emacs: teach elisp builders the finalAttrs pattern #330589

Merged
merged 9 commits into from
Aug 15, 2024
Merged

emacs: teach elisp builders the finalAttrs pattern #330589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3b82c96
emacs: fix name when pname is overridden
jian-lin Jul 25, 2024
675dcef
emacs: remove redundant doCheck from genericBuild
jian-lin Jul 26, 2024
cdf4aef
emacs: handle propagatedBuildInputs correctly for genericBuild
jian-lin Jul 28, 2024
84c2e00
emacs: stop generating autoload file for trivialBuild
jian-lin Jul 28, 2024
37df73d
emacs: teach elisp builders the finalAttrs pattern
jian-lin Jul 28, 2024
5248f6f
emacs: stop vendoring PR #234651
jian-lin Aug 4, 2024
5805cf2
emacs: remove unused parameters to make nixf-tidy-review bot happy
jian-lin Aug 4, 2024
e64ccec
emacs: make elpa2nix of elpaBuild consistent with melpaBuild
jian-lin Aug 4, 2024
bdd7734
emacs: inherit files in melpaBuild to make nixf-tidy CI happy
jian-lin Aug 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions pkgs/applications/editors/emacs/build-support/elpa.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,29 @@
{ lib, stdenv, emacs, texinfo, writeText }:

let
handledArgs = [ "meta" ];
genericBuild = import ./generic.nix { inherit lib stdenv emacs texinfo writeText; };
libBuildHelper = import ./lib-build-helper.nix;

in

libBuildHelper.extendMkDerivation' genericBuild (finalAttrs:

{ pname
, version
, src
, dontUnpack ? true
, meta ? {}
, ...
}@args:

genericBuild ({
{

elpa2nix = args.elpa2nix or ./elpa2nix.el;

dontUnpack = true;
inherit dontUnpack;

installPhase = ''
installPhase = args.installPhase or ''
runHook preInstall

emacs --batch -Q -l ${./elpa2nix.el} \
emacs --batch -Q -l "$elpa2nix" \
-f elpa2nix-install-package \
"$src" "$out/share/emacs/site-lisp/elpa"

Expand All @@ -34,4 +37,4 @@ genericBuild ({
} // meta;
}

// removeAttrs args handledArgs)
)
32 changes: 16 additions & 16 deletions pkgs/applications/editors/emacs/build-support/generic.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

let
inherit (lib) optionalAttrs;
handledArgs = [ "buildInputs" "nativeBuildInputs" "packageRequires" "propagatedUserEnvPkgs" "meta" ]
++ lib.optionals (emacs.withNativeCompilation or false) [ "postInstall" ];

setupHook = writeText "setup-hook.sh" ''
source ${./emacs-funcs.sh}
Expand All @@ -21,13 +19,16 @@ let
fi
'';

libBuildHelper = import ./lib-build-helper.nix;

in

{ pname
, version
, buildInputs ? []
libBuildHelper.extendMkDerivation' stdenv.mkDerivation (finalAttrs:

jian-lin marked this conversation as resolved.
Show resolved Hide resolved
{ buildInputs ? []
, nativeBuildInputs ? []
, packageRequires ? []
, propagatedBuildInputs ? []
, propagatedUserEnvPkgs ? []
, postInstall ? ""
, meta ? {}
Expand All @@ -36,10 +37,10 @@ in
, ...
}@args:

stdenv.mkDerivation (finalAttrs: ({
name = "emacs-${pname}-${finalAttrs.version}";
{
name = args.name or "emacs-${finalAttrs.pname}-${finalAttrs.version}";

unpackCmd = ''
unpackCmd = args.unpackCmd or ''
case "$curSrc" in
*.el)
# keep original source filename without the hash
Expand All @@ -55,14 +56,13 @@ stdenv.mkDerivation (finalAttrs: ({
esac
'';

buildInputs = packageRequires ++ buildInputs;
inherit packageRequires;
buildInputs = finalAttrs.packageRequires ++ buildInputs;
nativeBuildInputs = [ emacs texinfo ] ++ nativeBuildInputs;
propagatedBuildInputs = packageRequires;
propagatedUserEnvPkgs = packageRequires ++ propagatedUserEnvPkgs;

inherit setupHook;
propagatedBuildInputs = finalAttrs.packageRequires ++ propagatedBuildInputs;
propagatedUserEnvPkgs = finalAttrs.packageRequires ++ propagatedUserEnvPkgs;

doCheck = false;
setupHook = args.setupHook or setupHook;

meta = {
broken = false;
Expand All @@ -74,7 +74,7 @@ stdenv.mkDerivation (finalAttrs: ({

// optionalAttrs (emacs.withNativeCompilation or false) {

addEmacsNativeLoadPath = true;
addEmacsNativeLoadPath = args.addEmacsNativeLoadPath or true;

inherit turnCompilationWarningToError ignoreCompilationError;

Expand All @@ -97,4 +97,4 @@ stdenv.mkDerivation (finalAttrs: ({
'' + postInstall;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost all other attrs are overridden by user-defined values. Applying that pattern to postInstall means changing postInstall = '' .... '' + postInstall to postInstall = args.postInstall or '' ... '', which means if users define postInstall, their package never does native compilation. I think this is very bad. However, inconsistence is also not good. WDYT?

Copy link
Member

@AndersonTorres AndersonTorres Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well, well, well...

Searching some packages, I have found that e.g. copyDesktopItems injects itself at postInstallHooks.

postInstallHooks+=(copyDesktopItems)
# rest of code

It implies that post installation hooks are "queued" but not destroyed/substituted.
(And it explains why we need to never forget the runHook pre<phase-name>...)

That being said, curiously the consistency is not in substituting pre/post phases, but in queueing them.
So I vote for + instead of or.

Copy link
Contributor Author

@jian-lin jian-lin Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note that there are two kinds of postInstall hooks, one is defined in nix files with the attr postInstall, others are defined as an array called postInstallHooks in setup hooks. For the first kind of hook, it is indeed destroyed/substituted. For the second kind of hook, generally it is queued using preInstallHooks+=(...). Here, we only talk about the first kind of hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another question, which one of the two orders should be used for +, postInstall = '' .... '' + postInstall or postInstall = postInstall + '' .... ''?

}

// removeAttrs args handledArgs))
)
5 changes: 5 additions & 0 deletions pkgs/applications/editors/emacs/build-support/lib-build-helper.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
extendMkDerivation' =
mkDerivationBase: attrsOverlay: fpargs:
(mkDerivationBase fpargs).overrideAttrs attrsOverlay;
}
32 changes: 18 additions & 14 deletions pkgs/applications/editors/emacs/build-support/melpa.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
{ lib, stdenv, fetchFromGitHub, emacs, texinfo, writeText }:

let
handledArgs = [ "meta" "preUnpack" "postUnpack" ];
genericBuild = import ./generic.nix { inherit lib stdenv emacs texinfo writeText; };
libBuildHelper = import ./lib-build-helper.nix;

packageBuild = stdenv.mkDerivation {
name = "package-build";
Expand All @@ -29,6 +29,8 @@ let

in

libBuildHelper.extendMkDerivation' genericBuild (finalAttrs:

{ /*
pname: Nix package name without special symbols and without version or
"emacs-" prefix.
Expand All @@ -51,7 +53,7 @@ in
This will be written into the generated package but it is not needed during
the build process.
*/
, commit ? (args.src.rev or "unknown")
, commit ? (finalAttrs.src.rev or "unknown")
/*
files: Optional recipe property specifying the files used to build the package.
If null, do not set it in recipe, keeping the default upstream behaviour.
Expand All @@ -62,24 +64,26 @@ in
recipe: Optional MELPA recipe.
Default: a minimally functional recipe
*/
, recipe ? (writeText "${pname}-recipe" ''
(${ename} :fetcher git :url ""
${lib.optionalString (files != null) ":files ${files}"})
, recipe ? (writeText "${finalAttrs.pname}-recipe" ''
Copy link
Member

Choose a reason for hiding this comment

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

Not something to address here and now:
Do we really need to use writeText for the recipe? Couldn't it be done inline in the builder instead, skipping the intermediate derivation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I created #334888 to track this potential improvement.

(${finalAttrs.ename} :fetcher git :url ""
${lib.optionalString (finalAttrs.files != null) ":files ${finalAttrs.files}"})
jian-lin marked this conversation as resolved.
Show resolved Hide resolved
'')
, preUnpack ? ""
, postUnpack ? ""
, meta ? {}
, ...
}@args:

genericBuild ({
{

elpa2nix = args.elpa2nix or ./elpa2nix.el;
melpa2nix = args.melpa2nix or ./melpa2nix.el;

elpa2nix = ./elpa2nix.el;
melpa2nix = ./melpa2nix.el;
inherit commit ename files recipe;

inherit packageBuild commit ename recipe;
packageBuild = args.packageBuild or packageBuild;

melpaVersion =
melpaVersion = args.melpaVersion or (
let
parsed = lib.flip builtins.match version
# match <version>-unstable-YYYY-MM-DD format
Expand All @@ -90,7 +94,7 @@ genericBuild ({
in
if unstableVersionInNixFormat
then date + "." + time
else version;
else finalAttrs.version);

preUnpack = ''
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost all other attrs are overridden by user-defined values. Should we apply that pattern to preUnpack for consistency, i.e., changing preUnpack = '' ... '' + preUnpack to preUnpack = args.preUnpack or '' ... ''? The same question applies to postUnpack.

Copy link
Member

Choose a reason for hiding this comment

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

preUnpack suggests the idea we need to some preparation to the source so that it can be unpacked by the unpack phase of the builder we are dealing with. Further, this unpack phase is, at least conceptually, a black box.

Think in something like "oh the unpacker does not like dashes in the filename, let's change it to underscores first!" The preUnpack would change the filename, so that it can be passed to the black box.

So I would follow the same as I said to postInstall: + instead of or.

mkdir -p "$NIX_BUILD_TOP/recipes"
Expand All @@ -108,7 +112,7 @@ genericBuild ({
ln -s "$NIX_BUILD_TOP/$sourceRoot" "$NIX_BUILD_TOP/working/$ename"
'' + postUnpack;

buildPhase = ''
buildPhase = args.buildPhase or ''
runHook preBuild

cd "$NIX_BUILD_TOP"
Expand All @@ -122,7 +126,7 @@ genericBuild ({
runHook postBuild
'';

installPhase = ''
installPhase = args.installPhase or ''
runHook preInstall

archive="$NIX_BUILD_TOP/packages/$ename-$melpaVersion.el"
Expand All @@ -143,4 +147,4 @@ genericBuild ({
} // meta;
}

// removeAttrs args handledArgs)
)
15 changes: 10 additions & 5 deletions pkgs/applications/editors/emacs/build-support/trivial.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,32 @@

{ callPackage, lib, ... }@envargs:

let
libBuildHelper = import ./lib-build-helper.nix;
in

libBuildHelper.extendMkDerivation' (callPackage ./generic.nix envargs) (finalAttrs:

args:

callPackage ./generic.nix envargs ({
buildPhase = ''
{
buildPhase = args.buildPhase or ''
runHook preBuild

emacs -L . --batch -f batch-byte-compile *.el

runHook postBuild
'';

installPhase = ''
installPhase = args.installPhase or ''
runHook preInstall

LISPDIR=$out/share/emacs/site-lisp
install -d $LISPDIR
install *.el *.elc $LISPDIR
emacs --batch -l package --eval "(package-generate-autoloads \"${args.pname}\" \"$LISPDIR\")"

runHook postInstall
'';
}

// args)
)