Skip to content

rustPlatform: improve composability when the other builder defines a phase #334476

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

Draft
jian-lin wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

rustPlatform: improve composability when the other builder defines a phase #334476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2cff9a
rust: WIP improve composiability when the other builder defines a phase
jian-lin Aug 13, 2024
153dcf6
emacsPackages.lspce: rewrite it as a single derivation
jian-lin Aug 13, 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
40 changes: 0 additions & 40 deletions pkgs/applications/editors/emacs/elisp-packages/manual-packages/lspce/module.nix
View file
Open in desktop

This file was deleted.

102 changes: 92 additions & 10 deletions pkgs/applications/editors/emacs/elisp-packages/manual-packages/lspce/package.nix
View file
Open in desktop
Copy link
Contributor Author

@jian-lin jian-lin Aug 15, 2024

Choose a reason for hiding this comment

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

Thread for #334476 (comment):

I think the solution is not clean. It looks like the unnamed Monster of Frankenstein to me. It is more a structural problem than any other thing.

@AndersonTorres Could you elaborate why you think it is not clean and what the structural problem is?

Copy link
Contributor Author

@jian-lin jian-lin Aug 17, 2024

Choose a reason for hiding this comment

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

One problem I see is that the current design of builders does not allow us to move phases. An example is that elisp packages with a Rust module want to do the elisp buildPhase after the Rust buildPhase and installPhase.

Copy link
Member

@AndersonTorres AndersonTorres Aug 17, 2024

Choose a reason for hiding this comment

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

This is too subjective and strongly tied to a very particular perception I have.

If I was in a more barebones, Slackware-like distro, I would just manually install the dependencies (Rust compiler, Emacs, libs etc.) and write a shell script that executes a sequence of commands in the pristine, upstream source code.

I would design such a build script in a pipeline fashion.
A rough and not very accurate sketch would be something like this:

  1. split the pristine source code in a Rust-only part and an Emacs-only part
  2. run the Rust compiler pipeline to transform the Rust part in object code
    • or, more accurately, something suitable be processed by the future Emacs pipeline
  3. merge the object code with the Emacs code
  4. run the Emacs pipeline to transform this Emacs+object-code to a final Elisp package

On the other hand, when I look to your solution above, it is something similar to having two pipelines, one made for Emacs and other for Rust, disassemble both and amending specific parts of each so that they look externally similar to a Emacs pipeline.

Indeed, stretching the analogy a little bit, you are effectively debasing and polishing the pipes from the Rust pipeline so that they can be more easily amended to other pipes.

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

you are effectively debasing and polishing the pipes from the Rust pipeline so that they can be more easily amended to other pipes

Yes, this is exactly what I am doing here, following the line of work making the Rust builder use setup hooks by @ danieldk. We share the same motivation: #112438.

I would say this is a way to make builders composable with minimal changes.

Original file line number Diff line number Diff line change
@@ -1,34 +1,116 @@
{
lib,
callPackage,
f,
markdown-mode,
melpaBuild,
nix-update-script,
yasnippet,
fetchFromGitHub,
rustPlatform,
}:

let
lspce-module = callPackage ./module.nix { };
src = fetchFromGitHub {
owner = "zbelial";
repo = "lspce";
rev = "fd320476df89cfd5d10f1b70303c891d3b1e3c81";
hash = "sha256-KnERYq/CvJhJIdQkpH/m82t9KFMapPl+CyZkYyujslU=";
};
in
melpaBuild {
pname = "lspce";
inherit (lspce-module) version src meta;
version = "1.1.0-unstable-2024-07-14";

inherit src;

packageRequires = [
f
markdown-mode
yasnippet
];

# to compile lspce.el, it needs lspce-module.so
files = ''(:defaults "${lib.getLib lspce-module}/lib/lspce-module.*")'';
cargoDeps = rustPlatform.fetchCargoTarball {
inherit src;
hash = "sha256-BMbkdWsVSXRNt8kqOs05376cGnGnivHGvEugX0p3bVc=";
};

nativeBuildInputs = [
rustPlatform.cargoSetupHook
rustPlatform.cargoBuildHook
rustPlatform.cargoInstallHook
rustPlatform.cargoCheckHook
];

cargoBuildType = "release";

# Copy the default buildPhase here and switch cd to pushd/popd in
# order to not cause rebuild of other Emacs lisp packages.
buildPhase = ''
runHook preBuild

pushd "$NIX_BUILD_TOP"

emacs --batch -Q \
-L "$NIX_BUILD_TOP/package-build" \
-l "$melpa2nix" \
-f melpa2nix-build-package \
$ename $melpaVersion $commit

popd

runHook postBuild
'';

# Copy the default installPhase here because adding the dynamic
# module to the package tarball needs to be done at a specific time,
# i.e., between cargoInstallHook and elpa2nix-install-package.
# It cannot be done in preInstall because preInstall, as an implicit
# string hook, runs before cargoInstallHook, as a hook in
# preInstallHooks.
installPhase = ''
runHook preInstall

archive="$NIX_BUILD_TOP/packages/$ename-$melpaVersion.el"
if [ ! -f "$archive" ]; then
archive="$NIX_BUILD_TOP/packages/$ename-$melpaVersion.tar"
fi

echo "add the dynamic module to the package tarball because it is needed for compilation"
Copy link
Member

@AndersonTorres AndersonTorres Aug 14, 2024

Choose a reason for hiding this comment

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

Suggested change
echo "add the dynamic module to the package tarball because it is needed for compilation"
# add the dynamic module to the package tarball because it is needed for compilation

I can't see a reason to echo this info by default.
If there is some reason to do this, it should use some logging functionality instead.

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.

I slightly prefer logging this info because it is a bit "unusual".

logging functionality

You mean things like nixInfoLog, right? It is very weird that those functions are not used outside of pkgs/stdenv/generic/setup.sh.

Copy link
Member

@AndersonTorres AndersonTorres Aug 14, 2024

Choose a reason for hiding this comment

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

They were merged into Staging a week ago: #331560

And were injected on Master yesterday: #332764

# rename module without changing suffix
# use for loop because there seem to be two modules on darwin systems
# https://github.com/zbelial/lspce/issues/7#issue-1783708570
tmp_content_directory=$ename-$melpaVersion
mkdir $tmp_content_directory
for f in $out/lib/*; do
mv --verbose $f $tmp_content_directory/lspce-module.''${f##*.}
tar --verbose --append --file=$archive $tmp_content_directory/lspce-module.''${f##*.}
done
rmdir --verbose $out/lib
unset tmp_content_directory

emacs --batch -Q \
-l "$elpa2nix" \
-f elpa2nix-install-package \
"$archive" "$out/share/emacs/site-lisp/elpa"

runHook postInstall
'';

doCheck = true;
cargoCheckType = "release";
checkFlags = [
# flaky test
"--skip=msg::tests::serialize_request_with_null_params"
];

passthru = {
inherit lspce-module;
updateScript = nix-update-script {
attrPath = "emacsPackages.lspce.lspce-module";
extraArgs = [ "--version=branch" ];
};
updateScript = nix-update-script { extraArgs = [ "--version=branch" ]; };
};

meta = {
homepage = "https://github.com/zbelial/lspce";
description = "LSP Client for Emacs implemented as a module using Rust";
license = lib.licenses.gpl3Only;
maintainers = with lib.maintainers; [ AndersonTorres ];
};
}
20 changes: 14 additions & 6 deletions pkgs/build-support/rust/hooks/cargo-build-hook.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ declare -a cargoBuildFlags
cargoBuildHook() {
echo "Executing cargoBuildHook"

runHook preBuild

# Let stdenv handle stripping, for consistency and to not break
# separateDebugInfo.
export "CARGO_PROFILE_${cargoBuildType@U}_STRIP"=false
Expand Down Expand Up @@ -50,11 +48,21 @@ cargoBuildHook() {
popd
fi

runHook postBuild

echo "Finished cargoBuildHook"
}

if [ -z "${dontCargoBuild-}" ] && [ -z "${buildPhase-}" ]; then
buildPhase=cargoBuildHook
cargoBuildPhase() {
runHook preBuild

cargoBuildHook

runHook postBuild
}

if [ -z "${dontCargoBuild-}" ]; then
if [ -z "${buildPhase-}" ]; then
buildPhase=cargoBuildPhase
else
preBuildHooks+=(cargoBuildHook)
fi
fi
16 changes: 12 additions & 4 deletions pkgs/build-support/rust/hooks/cargo-check-hook.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ declare -a cargoTestFlags
cargoCheckHook() {
echo "Executing cargoCheckHook"

runHook preCheck

if [[ -n "${buildAndTestSubdir-}" ]]; then
pushd "${buildAndTestSubdir}"
fi
Expand Down Expand Up @@ -46,10 +44,20 @@ cargoCheckHook() {
fi

echo "Finished cargoCheckHook"
}

cargoCheckPhase() {
runHook preCheck

cargoCheckHook

runHook postCheck
}

if [ -z "${dontCargoCheck-}" ] && [ -z "${checkPhase-}" ]; then
checkPhase=cargoCheckHook
if [ -z "${dontCargoCheck-}" ]; then
if [ -z "${checkPhase-}" ]; then
checkPhase=cargoCheckPhase
else
preCheckHooks+=(cargoCheckHook)
fi
fi
21 changes: 15 additions & 6 deletions pkgs/build-support/rust/hooks/cargo-install-hook.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ cargoInstallPostBuildHook() {
cargoInstallHook() {
echo "Executing cargoInstallHook"

runHook preInstall

# rename the output dir to a architecture independent one

releaseDir=target/@targetSubdirectory@/$cargoBuildType
Expand All @@ -37,13 +35,24 @@ cargoInstallHook() {
-regex ".*\.\(so.[0-9.]+\|so\|a\|dylib\)" \
-print0 | xargs -r -0 cp -t $out/lib
rmdir --ignore-fail-on-non-empty $out/lib $out/bin
runHook postInstall

echo "Finished cargoInstallHook"
}

cargoInstallPhase() {
runHook preInstall

cargoInstallHook

runHook postInstall
}


if [ -z "${dontCargoInstall-}" ] && [ -z "${installPhase-}" ]; then
installPhase=cargoInstallHook
postBuildHooks+=(cargoInstallPostBuildHook)
if [ -z "${dontCargoInstall-}" ]; then
if [ -z "${installPhase-}" ]; then
installPhase=cargoInstallPhase
else
preInstallHooks+=(cargoInstallHook)
fi
postBuildHooks+=(cargoInstallPostBuildHook)
fi