emacs: fix for darwin's aarch64 codesigning issue #124888

hlolli · 2021-05-29T18:51:35Z

Motivation for this change

Excited to get started with aarch64 but Emacs doesn't compile
as well does #124571 not fix the issue I have for me.

Things done

Felt lucky to have bumped into this commit emacs-mirror/emacs@868f513 so the work has been done for us.

As well I remove the osx upper limit, I think it's fair to say that we are ok making assumtions about recency of aarch64 apple m1 computers. But please correct me if I'm wrong.

Screen:
built with nix-build -E 'with import <nixpkgs> { localSystem = "aarch64-darwin"; }; (import ./default.nix { localSystem = "aarch64-darwin"; }).emacs' -o result

built with nix-build -E 'with import <nixpkgs> { localSystem = "x86_64-darwin"; }; (import ./default.nix { localSystem = "x86_64-darwin"; }).emacs' -o result

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Added a release notes entry if the change is major or breaking
Fits CONTRIBUTING.md.

hlolli · 2021-05-29T18:53:04Z

pkgs/applications/editors/emacs/generic.nix

  postPatch = lib.concatStringsSep "\n" [
    (lib.optionalString srcRepo ''
      rm -fr .git
    '')

    # Reduce closure size by cleaning the environment of the emacs dumper
-    ''
+    (lib.optionalString ((stdenv.isDarwin && stdenv.isAarch64) == false) ''


I think this is a red herring, I'm going to test if this works without this change...

not a red herring, needs to be there

I don't see a failure if I remove this change. What error are you seeing?

double blunder, this really isn't needed, now removed.

pkgs/applications/editors/emacs/generic.nix

purcell

This looks good to me

thefloweringash

Mostly seems reasonable, just a couple of comments.

thefloweringash · 2021-05-31T08:08:38Z

pkgs/applications/editors/emacs/generic.nix

@@ -46,10 +46,10 @@ assert withXwidgets -> withGTK3 && webkitgtk != null;
 let emacs = stdenv.mkDerivation (lib.optionalAttrs nativeComp {
  NATIVE_FULL_AOT = "1";
  LIBRARY_PATH = "${lib.getLib stdenv.cc.libc}/lib";
-} // lib.optionalAttrs stdenv.isDarwin {
+} // lib.optionalAttrs (stdenv.isDarwin && !stdenv.isAarch64) {


This CFLAG was added in #41181, but I can't find the exact reason why this was added. It is it something we still need or can we drop it on all platforms? This controls the availability macros, and since we build against the SDK corresponding to our minimum version, this flag should have no effect.

If we keep it, it should probably follow the SDK version on all platforms, like in #124571.

Good catch, I build it with nix-build -E 'with import <nixpkgs> { localSystem = "x86_64-darwin"; }; (import ./default.nix { localSystem = "x86_64-darwin"; }).emacs' -o result and the binary (.App bundle) ran perfectly fine. Now removed for both platforms.

thefloweringash · 2021-05-31T08:16:52Z

pkgs/applications/editors/emacs/generic.nix

+
+  # Emacs checks for arm-apple-darwin, but our configuration is
+  # aarch64-apple-darwin. Force enable DO_CODESIGN.
+  preConfigure = lib.optionalString (stdenv.isDarwin && stdenv.isAarch64) ''


I looked at this in more detail, and it looks like emacs itself has been churning between arm and aarch64, but the most recent commit uses aarch64. If we backport this change as a patch then we know when to remove it (when it conflicts).

Ok, should we change the comment, or keep it like this?

Hmm. Add a link to the upstream change for context. This sed is probably the easiest way to make the required change.

Add a link to the upstream change for context.

^ I think this suggestion is still open, @hlolli. 🙂

Looking forward to these changes getting merged.

Thanks for reminding me! I misunderstood as well. This part I copy pasted from the @thefloweringash's previous wip PR. Anyway, late for me here, tomorrow!

ok I pushed a fetchpatch call to the svannah commit. Which should error when the patch doesn't apply anymore!

pkgs/applications/editors/emacs/generic.nix

thefloweringash · 2021-06-13T15:04:14Z

Change LGTM. The commit history needs tidying. See the guidelines for commit messages.

hlolli · 2021-06-15T14:06:44Z

Change LGTM. The commit history needs tidying. See the guidelines for commit messages.

fixed

github-actions · 2021-06-15T18:48:12Z

Backport failed for release-21.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally:

git fetch origin release-21.05
git worktree add -d .worktree/backport-124888-to-release-21.05 origin/release-21.05
cd .worktree/backport-124888-to-release-21.05
git checkout -b backport-124888-to-release-21.05
ancref=$(git merge-base 16b157c3462f547e989faf7a74203bf0f44a9c94 5efbcf8d7eef8a6380dafb9fbfb6420b1e051da9)
git cherry-pick -x $ancref..5efbcf8d7eef8a6380dafb9fbfb6420b1e051da9

github-actions · 2021-06-15T18:48:59Z

Backport failed for release-21.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally:

git fetch origin release-21.05
git worktree add -d .worktree/backport-124888-to-release-21.05 origin/release-21.05
cd .worktree/backport-124888-to-release-21.05
git checkout -b backport-124888-to-release-21.05
ancref=$(git merge-base 16b157c3462f547e989faf7a74203bf0f44a9c94 5efbcf8d7eef8a6380dafb9fbfb6420b1e051da9)
git cherry-pick -x $ancref..5efbcf8d7eef8a6380dafb9fbfb6420b1e051da9

hlolli requested a review from adisbladis as a code owner May 29, 2021 18:51

ofborg bot added the 6.topic: darwin Running or building packages on Darwin label May 29, 2021

github-actions bot added the 6.topic: emacs Text editor label May 29, 2021

hlolli commented May 29, 2021

View reviewed changes

ofborg bot requested review from lovek323, peti and jwiegley May 29, 2021 19:01

ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels May 29, 2021

hlolli changed the title ~~emacs: add upstream patch for emacs fix of darwin's aarch64 codesigning issue~~ emacs: emacs fix for darwin's aarch64 codesigning issue May 29, 2021

hlolli changed the title ~~emacs: emacs fix for darwin's aarch64 codesigning issue~~ emacs: fix for darwin's aarch64 codesigning issue May 29, 2021

ofborg bot added 10.rebuild-linux: 11-100 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels May 29, 2021

veprbl reviewed May 29, 2021

View reviewed changes

pkgs/applications/editors/emacs/generic.nix Outdated Show resolved Hide resolved

veprbl requested a review from thefloweringash May 29, 2021 21:03

hlolli mentioned this pull request May 30, 2021

Using the new native M1 support LnL7/nix-darwin#334

Closed

purcell approved these changes May 30, 2021

View reviewed changes

thefloweringash reviewed May 31, 2021

View reviewed changes

hlolli force-pushed the aarch64-darwin/fix-emacs-codesigning branch from ef3a770 to 91278c1 Compare May 31, 2021 17:54

ofborg bot added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jun 10, 2021

matthewbauer approved these changes Jun 12, 2021

View reviewed changes

matthewbauer reviewed Jun 12, 2021

View reviewed changes

pkgs/applications/editors/emacs/generic.nix Show resolved Hide resolved

emacs: fix darwin's aarch64 codesigning issue

5efbcf8

hlolli force-pushed the aarch64-darwin/fix-emacs-codesigning branch from 1ac6f63 to 5efbcf8 Compare June 15, 2021 14:06

domenkozar merged commit 97ee9d3 into NixOS:master Jun 15, 2021

domenkozar added the backport release-21.05 label Jun 15, 2021

hlolli deleted the aarch64-darwin/fix-emacs-codesigning branch June 15, 2021 20:59

ghost mentioned this pull request Jun 16, 2021

Can not build emacsGcc in aarch64-darwin nix-community/emacs-overlay#162

Closed

hlolli mentioned this pull request Apr 19, 2022

emacs: fix build on aarch64-darwin #124571

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

emacs: fix for darwin's aarch64 codesigning issue #124888

emacs: fix for darwin's aarch64 codesigning issue #124888

hlolli commented May 29, 2021 •

edited

Loading

hlolli May 29, 2021

hlolli May 29, 2021

thefloweringash May 31, 2021 •

edited

Loading

hlolli May 31, 2021

purcell left a comment

thefloweringash left a comment

thefloweringash May 31, 2021

hlolli May 31, 2021

thefloweringash May 31, 2021

hlolli May 31, 2021

thefloweringash Jun 2, 2021

purcell Jun 9, 2021

hlolli Jun 9, 2021

hlolli Jun 10, 2021

thefloweringash commented Jun 13, 2021

hlolli commented Jun 15, 2021

github-actions bot commented Jun 15, 2021

github-actions bot commented Jun 15, 2021

emacs: fix for darwin's aarch64 codesigning issue #124888

emacs: fix for darwin's aarch64 codesigning issue #124888

Conversation

hlolli commented May 29, 2021 • edited Loading

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefloweringash May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purcell left a comment

Choose a reason for hiding this comment

thefloweringash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefloweringash commented Jun 13, 2021

hlolli commented Jun 15, 2021

github-actions bot commented Jun 15, 2021

github-actions bot commented Jun 15, 2021

hlolli commented May 29, 2021 •

edited

Loading

thefloweringash May 31, 2021 •

edited

Loading