emacs: parameterize native compilation patch #353137
Conversation
nix-owners
bot
requested review from
adisbladis,
jian-lin,
matthewbauer and
ttuegel
ofborg
bot
added
11.by: package-maintainer
| mkArgs = { pname, version, variant, patches ? _: [ ], rev, hash }: { | ||
| inherit pname version variant patches; | ||
| mkArgs = { pname, version, variant, templateNativeCompDriverOptionsPatch, patches ? _: [ ], rev, hash }: { | ||
| inherit pname version variant templateNativeCompDriverOptionsPatch patches; |
There was a problem hiding this comment.
This specific patch changes almost every release so I agree with the idea of moving it to mkArgs from make-emacs.nix. However, not putting it in patches seems a bit over-engineering to me. Just my 2 cents.
There was a problem hiding this comment.
Well, given that we are passing the same patch to the same version of both emacs and emacs-macport, now I think it is better to not moving the patch into mkArgs from make-emacs.nix.
There was a problem hiding this comment.
Why not?
The patch depends on the Emacs source being built.
It looks reasonable to parameterize it.
This is similar to the patches we are using for Emacs 28.
There was a problem hiding this comment.
Why not?
To reduces code duplication.
Thinking it a bit more, I am fine with keeping it in make-emacs.nix or putting it into patches of mkArgs.
| patches = patches fetchpatch ++ lib.optionals withNativeCompilation [ | ||
| (substituteAll { | ||
| src = if lib.versionOlder finalAttrs.version "29" | ||
| then ./native-comp-driver-options-28.patch | ||
| else if lib.versionOlder finalAttrs.version "30" | ||
| then ./native-comp-driver-options.patch | ||
| else ./native-comp-driver-options-30.patch; | ||
| backendPath = (lib.concatStringsSep " " | ||
| (builtins.map (x: ''"-B${x}"'') ([ | ||
| # Paths necessary so the JIT compiler finds its libraries: | ||
| "${lib.getLib libgccjit}/lib" | ||
| ] ++ libGccJitLibraryPaths ++ [ | ||
| # Executable paths necessary for compilation (ld, as): | ||
| "${lib.getBin stdenv.cc.cc}/bin" | ||
| "${lib.getBin stdenv.cc.bintools}/bin" | ||
| "${lib.getBin stdenv.cc.bintools.bintools}/bin" | ||
| ]))); | ||
| }) | ||
| ]; | ||
| patches = patches fetchpatch ++ lib.optionals withNativeCompilation [ nativeCompilationPatch ]; |
There was a problem hiding this comment.
Factoring out substituteAll seems over-engineering to me. It also makes the diff larger than needed and harder to review.
There was a problem hiding this comment.
Hmm, formatting makes the diff larger than needed and harder to review. Could you keep the old format? We can do formatting in a future PR or leave it to the tree-wide one.
AndersonTorres
force-pushed
the
emacs-native-compilation-patch
branch
from
72fa114
to
e0a0fad
Compare
Split from #341656
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.