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

nwjs: 0.54.1 -> 0.82.0 #262424

Merged
merged 6 commits into from
Dec 9, 2023
Merged

nwjs: 0.54.1 -> 0.82.0 #262424

merged 6 commits into from
Dec 9, 2023

Conversation

MikaelFangel
Copy link
Contributor

@MikaelFangel MikaelFangel commented Oct 21, 2023

Description of changes

This updates nwjs and as a noteworthy change I've added the autoPatchElfHook to the nativeBuildInputs, because of issues with running nwjs else wise. I've also run a formatter on the derivation, hence the many changes.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 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.

@MikaelFangel MikaelFangel marked this pull request as draft October 21, 2023 00:52
@MikaelFangel MikaelFangel added 1.severity: security Issues which raise a security issue, or PRs that fix one 8.has: package (update) This PR updates a package to a newer version labels Oct 21, 2023
@ofborg ofborg bot requested a review from offlinehacker October 21, 2023 06:15
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 21, 2023
@MikaelFangel
Copy link
Contributor Author

Result of nixpkgs review:

10 packages built: betaflight-configurator emuflight-configurator gridtracker inav-configurator nwjs nwjs-sdk onlykey pinegrow pinegrow6 popcorntime

@MikaelFangel MikaelFangel marked this pull request as ready for review October 21, 2023 16:33
@MikaelFangel MikaelFangel marked this pull request as draft October 21, 2023 16:36
@MikaelFangel
Copy link
Contributor Author

MikaelFangel commented Oct 21, 2023

Program runs:

  • popcorntime
  • pinegrow
  • pinegrow6
  • emuflight-configurator
  • betaflight-configurator
  • inav-configurator
  • gridtracker
  • onlykey
  • nwjs
  • nwjs-sdk

@MikaelFangel MikaelFangel added the 2.status: work-in-progress This PR isn't done label Oct 21, 2023
@MikaelFangel MikaelFangel removed the 2.status: work-in-progress This PR isn't done label Oct 21, 2023
@MikaelFangel MikaelFangel marked this pull request as ready for review October 21, 2023 18:13
@emilylange
Copy link
Member

Please move the formatting into a separate commit, thanks!

@MikaelFangel
Copy link
Contributor Author

@emilylange - Done (:

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

just some nits

pkgs/development/tools/nwjs/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/nwjs/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/nwjs/default.nix Outdated Show resolved Hide resolved
@MikaelFangel
Copy link
Contributor Author

MikaelFangel commented Oct 21, 2023

just some nits

@robert-manchester - Cool, I appreciate it 😃

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

i was playing around with this. have some comments inline about just removing the patchelf and for loops in the install phase and modifying the inputs to autoPatchelfHook to get a nice rpath.

[edit] just posted the changes in a PR

pkgs/development/tools/nwjs/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/nwjs/default.nix Outdated Show resolved Hide resolved
@MikaelFangel
Copy link
Contributor Author

Changes have been merged, and I've tested that programs using the library still run.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 22, 2023
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

i think makeWrapper can get removed.

pkgs/development/tools/nwjs/default.nix Show resolved Hide resolved
pkgs/development/tools/nwjs/default.nix Outdated Show resolved Hide resolved
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 22, 2023
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 22, 2023
@MikaelFangel MikaelFangel changed the title nwjs: 0.54.1 -> 0.81.0 nwjs: 0.54.1 -> 0.82.0 Nov 3, 2023
@MikaelFangel MikaelFangel removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 3, 2023
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Nov 6, 2023
MikaelFangel and others added 2 commits November 6, 2023 21:03
Co-authored-by: robert manchester <86313040+robert-manchester@users.noreply.github.com>
@nekowinston
Copy link
Contributor

nekowinston commented Dec 7, 2023

Works well enough on my GNOME setup, but Wayland on Sway should probably support NIXOS_OZONE_WL.
I managed to make it work like so:

{pkgs ? import (fetchTarball "https://github.com/MikaelFangel/nixpkgs-upstream/archive/update-nwjs.tar.gz") {}}:
pkgs.nwjs.overrideAttrs (oldAttrs: {
  buildInputs =
    (oldAttrs.buildInputs or [])
    ++ [
      (pkgs.wrapGAppsHook.override {
        makeWrapper = pkgs.makeWrapper;
      })
    ];
  preFixup = ''
    gappsWrapperArgs+=(
      --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--ozone-platform-hint=auto --enable-features=WaylandWindowDecorations}}"
    )
  '';
})

Without overriding makeWrapper, I get an error like

[1207/174457.298986:ERROR:zip.cc(179)] Cannot open '/home/winston/${NIXOS_OZONE_WL:+${WAYLAND_DISPLAY:+--ozone-platform-hint=auto': FILE_ERROR_NOT_FOUND: No such file or directory (2)

I looked to Signal for a workaround.

Just using makeWrapper leads to ./result/bin/nw: error while loading shared libraries: libgobject-2.0.so.0: cannot open shared object file: No such file or directory, so I assume wrapGAppsHook is needed.

@MikaelFangel
Copy link
Contributor Author

@nekowinston - The crash doesn't happen on Hyprland w/ wayland, but adding your suggestions makes nwjs respected by the WM. (:

@pbsds
Copy link
Member

pbsds commented Dec 9, 2023

Result of nixpkgs-review pr 262424 run on x86_64-linux 1

10 packages built:
  • betaflight-configurator
  • emuflight-configurator
  • gridtracker
  • inav-configurator
  • nwjs
  • nwjs-sdk
  • onlykey
  • pinegrow
  • pinegrow6
  • popcorntime

It is a joy to review large changes when it is so cleanly divided into atomic commits, thank you. Runs fine and LGTM!

@pbsds pbsds merged commit 45367f2 into NixOS:master Dec 9, 2023
21 of 22 checks passed
@MikaelFangel MikaelFangel deleted the update-nwjs branch December 9, 2023 21:46
@MikaelFangel
Copy link
Contributor Author

Thank you @pbsds (:

@nagy nagy mentioned this pull request Dec 10, 2023
12 tasks
@MikaelFangel MikaelFangel mentioned this pull request Dec 26, 2023
13 tasks
@MikaelFangel MikaelFangel added the 8.has: port to stable A PR already has a backport to the stable release. label Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 8.has: package (update) This PR updates a package to a newer version 8.has: port to stable A PR already has a backport to the stable release. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants