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

nyxt: fix darwin build #126082

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

Conversation

midchildan
Copy link
Member

@midchildan midchildan commented Jun 7, 2021

Motivation for this change

Continuation of #125113.

  • Fix the Darwin build
  • Use the GI-GTK backend as recommended by @jmercouris
  • Ran nixpkgs-fmt

Keyboard input is still not working, but it turned out to be an issue with WebKitGTK so I'm preparing a separate issue for it. Currently, all packages using WebKitGTK would have the same issue on Darwin.

ping @mjlbach

Things done
  • 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/)
  • 21.11 Release Notes
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jun 7, 2021
@midchildan midchildan mentioned this pull request Jun 7, 2021
9 tasks
@ofborg ofborg bot requested a review from nlewo June 7, 2021 15:19

GST_PLUGIN_SYSTEM_PATH_1_0 = lib.concatMapStringsSep ":" (p: "${p}/lib/gstreamer-1.0") gstBuildInputs;
Copy link
Member Author

Choose a reason for hiding this comment

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

Setting GST_PLUGIN_SYSTEM_PATH_1_0 is handled by wrapGApps. It suffices to put these in buildInputs.

glib gdk-pixbuf cairo
mime-types pango gtk3
glib-networking gsettings-desktop-schemas
xclip notify-osd enchant
Copy link
Member Author

Choose a reason for hiding this comment

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

xclip in buildInputs doesn't have any effect because it's a CLI tool, not a library. I've added xclip to the wrapper instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

notify-osd is removed since it's no longer needed. Details in #125113 (comment).

cp -f $src/lib/common-lisp/nyxt/assets/nyxt_''${i}x''${i}.png "$out/share/icons/hicolor/''${i}x''${i}/apps/nyxt.png"
done

mkdir -p $out/bin && makeWrapper $src/bin/nyxt $out/bin/nyxt \
Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't work because gappsWrapperArgs is computed at preFixup, which happens after installPhase.

Comment on lines -134 to -141
(
source "$out/lib/common-lisp-settings"/*-shell-config.sh
cd "$out/lib/common-lisp"/*/
makeFlags="''${makeFlags:-}"
make LISP=common-lisp.sh NYXT_INTERNAL_QUICKLISP=false PREFIX="$out" $makeFlags all
make LISP=common-lisp.sh NYXT_INTERNAL_QUICKLISP=false PREFIX="$out" $makeFlags install
cp nyxt "$out/bin/nyxt"
)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was redundant because it creates a broken build artifact $out/bin/nyxt which promptly gets rebuilt by the subsequent commands.

Comment on lines 161 to 167
nix_lisp_build_system nyxt/gi-gtk-application \
"(asdf/system:component-entry-point (asdf:find-system :nyxt/gi-gtk-application))" \
"" \
"(format *error-output* \"Alien objects:~%~s~%\" sb-alien::*shared-objects*)"
' "$out/bin/nyxt-lisp-launcher.sh"
cp "$out/lib/common-lisp/nyxt/nyxt" "$out/bin/"

mv "$out/lib/common-lisp/nyxt/nyxt" "$out/bin/"
Copy link
Member Author

Choose a reason for hiding this comment

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

Replaces the GTK backend with the GI-GTK one.

)

# clear unnecessary environment variables to avoid hitting the limit
env -i \
Copy link
Member Author

Choose a reason for hiding this comment

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

It's necessary to clear unnecessary environment variables because otherwise it hits the length limit imposed by the OS. This is similar to what is done in lisp-modules/define-package.nix:

env -i \
NIX_LISP="$NIX_LISP" \
NIX_LISP_PRELAUNCH_HOOK='nix_lisp_run_single_form "(progn
${lib.concatMapStrings (system: ''
(asdf:compile-system :${system})
(asdf:load-system :${system})
(asdf:operate (quote asdf::compile-bundle-op) :${system})
(ignore-errors (asdf:operate (quote asdf::deploy-asd-op) :${system}))
'') buildSystems}
)"' \
"$out/bin/${args.baseName}-lisp-launcher.sh"

@r-rmcgibbo
Copy link

r-rmcgibbo commented Jun 7, 2021

Result of nixpkgs-review pr 126082 at 0498cf00 run on x86_64-linux 1

1 package failed to build:
1 package built successfully:
  • lispPackages.nyxt
2 suggestions:
  • warning: missing-phase-hooks

    checkPhase should probably contain runHook preCheck and runHook postCheck.

    Near pkgs/applications/networking/browsers/nyxt/default.nix:82:3:

       |
    82 |   checkPhase = ''
       |   ^
    
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/applications/networking/browsers/nyxt/default.nix:51:3:

       |
    51 |   installPhase =
       |   ^
    

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.


Result of nixpkgs-review pr 126082 at 0498cf00 run on aarch64-linux 1

1 package failed to build:

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

@midchildan midchildan force-pushed the fix/nyxt-darwin branch 2 times, most recently from d2a2089 to 45f126d Compare June 7, 2021 16:07
@midchildan
Copy link
Member Author

midchildan commented Jun 7, 2021

I've pushed a few more fixes that does the following:

  • Add dontStrip = true to nyxt to fix the Linux build. Stripping results in a crash, likely because the resulting binary is already stripped once in lispPackages.nyxt.
  • Actually run the checkPhase, and make it installCheckPhase because it needs to run after installPhase. I've also disabled it for Darwin because the test would initialize the GTK runtime, which I suspect would fail on Hydra.
  • Add runHook as suggested by @r-rmcgibbo

@babariviere
Copy link
Contributor

Result of nixpkgs-review pr 126082 run on x86_64-darwin 1

2 packages built:
  • lispPackages.nyxt
  • nyxt

@mjlbach
Copy link
Contributor

mjlbach commented Jun 7, 2021

The changes look good to me:

  • As noted, keyboard input event don't work unless holding down cmd, which may be a webkitgtk issue
  • It's a bit crash-ey for me, I can't use it more than 5 minutes without a crash, but that's probably an upstream issue
  • It's still quite laggy

@midchildan midchildan requested a review from ryantm as a code owner June 7, 2021 18:46
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Jun 7, 2021
Co-authored-by: Michael Lingelbach <m.j.lbach@gmail.com>
@midchildan
Copy link
Member Author

midchildan commented Jun 7, 2021

I've now renamed lispPackages.nyxt to lispPackages.nyxt-unwrapped for more clarity and updated the release notes accordingly. This should have minimal effect because most users would have nyxt installed instead.

Also rebased to fix the merge conflict in the release notes.

@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package labels Jun 7, 2021
@github-actions github-actions bot removed 8.has: changelog 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation labels Jun 13, 2021
@ethancedwards8
Copy link
Member

Result of nixpkgs-review pr 126082 run on x86_64-darwin 1

1 package built:
  • nyxt

@stale
Copy link

stale bot commented Jan 8, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 8, 2022
@wegank wegank marked this pull request as draft February 2, 2023 09:01
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Feb 2, 2023
@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: darwin Running or building packages on Darwin 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants