rustPlatform: improve composability when the other builder defines a phase #334476
Conversation
Previously, if the other builder defines a phase, Rust hooks do nothing, which is not desirable. With this patch applied, if the other builder defines a phase, Rust hooks will use pre hoooks instead. TODO: maybe also change the other 3 hooks
Previously, this package is implemented as two derivations, which is inconvenient for overriding. This patch uses hooks from rustPlatform to implement it as a single derivation, which can be overridden like any other Rust packages.
| archive="$NIX_BUILD_TOP/packages/$ename-$melpaVersion.tar" | ||
| fi | ||
|
|
||
| echo "add the dynamic module to the package tarball because it is needed for compilation" |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
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.
It is better to not change the working directory. One example showing the benefit is the rewriting[1] of lspce, where the Rust build hook assumes the working directory is unchanged. [1]: NixOS#334476
|
Well, from my part I approve this PR because it improves things. On the other hand, I think the solution is not clean. It looks like the unnamed Monster of Frankenstein to me. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- split the pristine source code in a Rust-only part and an Emacs-only part
- 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
- merge the object code with the Emacs code
- 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.
There was a problem hiding this comment.
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.
It is better to not change the working directory. One example showing the benefit is the rewriting[1] of lspce, where the Rust build hook assumes the working directory is unchanged. [1]: NixOS/nixpkgs#334476
wegank
added
the
2.status: merge conflict
Description of changes
Motivation: improve composability between the rust builder and Emacs lisp builders
Previously, if the other builder defines a phase, Rust hooks do
nothing, which is not desirable. With this patch applied, if the
other builder defines a phase, Rust hooks will use pre hoooks instead.
Review notes
emacsPackages.lspceis an Emacs lisp package with a dynamic module written in Rust. The commit rewriting it is an example of the improvement this PR brings.emacsPackages.lspcebuilds.TODO
cargoNextestHookandmaturinBuildHook. I am not familiar with them though, so help is appreciated.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.