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

buildLuaPackage: enable __structuredAttrs rocks #224553

Merged
merged 4 commits into from
Apr 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
118 changes: 56 additions & 62 deletions pkgs/development/interpreters/lua-5/build-lua-package.nix
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{ lib
, lua
, wrapLua
, luarocks

# Whether the derivation provides a lua module or not.
, luarocksCheckHook
, luaLib
Expand Down Expand Up @@ -38,13 +38,7 @@

# Skip wrapping of lua programs altogether
, dontWrapLuaPrograms ? false

, meta ? {}

, passthru ? {}
, doCheck ? false
, doInstallCheck ? false

# Non-Lua / system (e.g. C library) dependencies. Is a list of deps, where
# each dep is either a derivation, or an attribute set like
# { name = "rockspec external_dependencies key"; dep = derivation; }
Expand Down Expand Up @@ -73,7 +67,6 @@
# Keep extra attributes from `attrs`, e.g., `patchPhase', etc.

let
generatedRockspecFilename = "${rockspecDir}/${pname}-${rockspecVersion}.rockspec";

# TODO fix warnings "Couldn't load rockspec for ..." during manifest
# construction -- from initial investigation, appears it will require
Expand All @@ -82,69 +75,76 @@ let
# configured trees)
luarocks_config = "luarocks-config.lua";

# Filter out the lua derivation itself from the Lua module dependency
# closure, as it doesn't have a rock tree :)
requiredLuaRocks = lib.filter (d: d ? luaModule)
(lua.pkgs.requiredLuaModules (luarocksDrv.nativeBuildInputs ++ luarocksDrv.propagatedBuildInputs));

# example externalDeps': [ { name = "CRYPTO"; dep = pkgs.openssl; } ]
externalDepsGenerated = lib.unique (lib.filter (drv: !drv ? luaModule) (
luarocksDrv.nativeBuildInputs ++ luarocksDrv.propagatedBuildInputs ++ luarocksDrv.buildInputs)
);
externalDeps' = lib.filter (dep: !lib.isDerivation dep) externalDeps;

luarocksDrv = luaLib.toLuaModule ( lua.stdenv.mkDerivation (finalAttrs: let

rocksSubdir = "${finalAttrs.pname}-${finalAttrs.version}-rocks";
luarocks_content = let
generatedConfig = luaLib.generateLuarocksConfig {
externalDeps = externalDeps ++ externalDepsGenerated;
inherit extraVariables rocksSubdir requiredLuaRocks;
};
in
''
${generatedConfig}
${extraConfig}
'';
in builtins.removeAttrs attrs ["disabled" "externalDeps" "extraVariables"] // {

name = namePrefix + pname + "-" + finalAttrs.version;
luarocksDrv = luaLib.toLuaModule ( lua.stdenv.mkDerivation (self: attrs // {

name = namePrefix + pname + "-" + self.version;
inherit rockspecVersion;

__structuredAttrs = true;
env = {
LUAROCKS_CONFIG="$PWD/${luarocks_config}";
};

generatedRockspecFilename = "${rockspecDir}/${pname}-${rockspecVersion}.rockspec";


nativeBuildInputs = [
wrapLua
luarocks
] ++ lib.optionals doCheck ([ luarocksCheckHook ] ++ finalAttrs.nativeCheckInputs);
lua.pkgs.luarocks
];

buildInputs = buildInputs
++ (map (d: d.dep) externalDeps');
inherit doCheck extraVariables rockspecFilename knownRockspec externalDeps nativeCheckInputs;

buildInputs = let
# example externalDeps': [ { name = "CRYPTO"; dep = pkgs.openssl; } ]
externalDeps' = lib.filter (dep: !lib.isDerivation dep) self.externalDeps;
in [ lua.pkgs.luarocks ]
++ lib.optionals self.doCheck ([ luarocksCheckHook ] ++ self.nativeCheckInputs)
++ (map (d: d.dep) externalDeps')
;

# propagate lua to active setup-hook in nix-shell
propagatedBuildInputs = propagatedBuildInputs ++ [ lua ];

# @-patterns do not capture formal argument default values, so we need to
# explicitly inherit this for it to be available as a shell variable in the
# builder
inherit rocksSubdir;
rocksSubdir = "${self.pname}-${self.version}-rocks";
luarocks_content = let
externalDepsGenerated = lib.filter (drv: !drv ? luaModule)
(self.nativeBuildInputs ++ self.propagatedBuildInputs ++ self.buildInputs);
generatedConfig = luaLib.generateLuarocksConfig {
externalDeps = lib.unique (self.externalDeps ++ externalDepsGenerated);
# Filter out the lua derivation itself from the Lua module dependency
# closure, as it doesn't have a rock tree :)
# luaLib.hasLuaModule
requiredLuaRocks = lib.filter luaLib.hasLuaModule
(lua.pkgs.requiredLuaModules (self.nativeBuildInputs ++ self.propagatedBuildInputs));
inherit (self) extraVariables rocksSubdir;
};
in
''
${generatedConfig}
${extraConfig}
'';

configurePhase = ''
runHook preConfigure

cat > ${luarocks_config} <<EOF
${luarocks_content}
${self.luarocks_content}
EOF
export LUAROCKS_CONFIG="$PWD/${luarocks_config}";
cat "$LUAROCKS_CONFIG"
''
+ lib.optionalString (rockspecFilename == null) ''
rockspecFilename="${generatedRockspecFilename}"
+ lib.optionalString (self.rockspecFilename == null) ''
rockspecFilename="${self.generatedRockspecFilename}"
''
+ lib.optionalString (knownRockspec != null) ''

+ lib.optionalString (self.knownRockspec != null) ''
# prevents the following type of error:
# Inconsistency between rockspec filename (42fm1b3d7iv6fcbhgm9674as3jh6y2sh-luv-1.22.0-1.rockspec) and its contents (luv-1.22.0-1.rockspec)
rockspecFilename="$TMP/$(stripHash ''${knownRockspec})"
cp ''${knownRockspec} "$rockspecFilename"
rockspecFilename="$TMP/$(stripHash ${self.knownRockspec})"
cp ${self.knownRockspec} "$rockspecFilename"
''
+ ''
runHook postConfigure
Expand All @@ -155,9 +155,9 @@ let

nix_debug "Using LUAROCKS_CONFIG=$LUAROCKS_CONFIG"

LUAROCKS=luarocks
LUAROCKS_EXTRA_ARGS=""
if (( ''${NIX_DEBUG:-0} >= 1 )); then
LUAROCKS="$LUAROCKS --verbose"
LUAROCKS_EXTRA_ARGS=" --verbose"
fi

runHook postBuild
Expand All @@ -167,7 +167,7 @@ let
wrapLuaPrograms
'' + attrs.postFixup or "";

installPhase = attrs.installPhase or ''
Copy link
Member

@arcnmx arcnmx Apr 24, 2023

Choose a reason for hiding this comment

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

I am sorry if this broke your workflow but the fixes are trivial in comparison

I understand it's an impactful/breaking change to the builder in general, the only part I really take issue with here is that no care or attention was given to preserving input attrs.

The lines with or/++/// like this one (and all the others: meta, passthru, buildInputs, installPhase, checkPhase, etc) were removed while they could've just been left as-is - this leaves the function incorrect and less flexible than it was before, for no benefit or change. It's just silly to remove lines unrelated to your changes that serve a purpose 😞

installPhase = ''
runHook preInstall

# work around failing luarocks test for Write access
Expand All @@ -182,21 +182,17 @@ let
# maybe we could reestablish dependency checking via passing --rock-trees

nix_debug "ROCKSPEC $rockspecFilename"
nix_debug "cwd: $PWD"
$LUAROCKS make --deps-mode=all --tree=$out ''${rockspecFilename}
luarocks $LUAROCKS_EXTRA_ARGS make --deps-mode=all --tree=$out ''${rockspecFilename}

runHook postInstall
'';


checkPhase = attrs.checkPhase or ''
checkPhase = ''
runHook preCheck
$LUAROCKS test
luarocks test
runHook postCheck
'';

LUAROCKS_CONFIG="$PWD/${luarocks_config}";

shellHook = ''
runHook preShell
export LUAROCKS_CONFIG="$PWD/${luarocks_config}";
Expand All @@ -205,16 +201,14 @@ let

passthru = {
inherit lua; # The lua interpreter
inherit externalDeps;
inherit luarocks_content;
} // passthru;
};
Copy link
Member

Choose a reason for hiding this comment

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

} // attrs.passthru or {};

Copy link
Member

Choose a reason for hiding this comment

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

also env is important to preserve too


meta = {
platforms = lua.meta.platforms;
# add extra maintainer(s) to every package
maintainers = (meta.maintainers or []) ++ [ ];
maintainers = (attrs.meta.maintainers or []) ++ [ ];
broken = disabled;
} // meta;
} // attrs.meta;
Copy link
Member

Choose a reason for hiding this comment

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

attrs.meta or {}

}));
in
luarocksDrv
5 changes: 2 additions & 3 deletions pkgs/development/lua-modules/lib.nix
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ rec {
so that luaRequireModules can be run later
*/
toLuaModule = drv:
drv.overrideAttrs( oldAttrs: {
drv.overrideAttrs(oldAttrs: {
# Use passthru in order to prevent rebuilds when possible.
passthru = (oldAttrs.passthru or {}) // {
luaModule = lua;
Expand All @@ -81,8 +81,7 @@ rec {
};
*/
generateLuarocksConfig = {
externalDeps

externalDeps
# a list of lua derivations
, requiredLuaRocks
, extraVariables ? {}
Expand Down
Loading