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

microsoft-edge-beta: init at 91.0.864.27 #123805

Closed
wants to merge 1 commit into from
Closed

Conversation

leo60228
Copy link
Member

Motivation for this change

Microsoft Edge is an increasingly popular web browser. Even if people don't use it themselves, developers on NixOS may wish to be able to test pages and extensions in it.

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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 123805 at b59cdb7 run on x86_64-linux 1

1 package built successfully:
  • microsoft-edge-beta
2 suggestions:
  • warning: name-and-version

    Did you mean to pass pname instead of name to mkDerivation?

    Near pkgs/applications/networking/browsers/microsoft-edge/default.nix:78:3:

       |
    78 |   name = "microsoft-edge${suffix}-${version}";
       |   ^
    

    Near pkgs/applications/networking/browsers/microsoft-edge/default.nix:76:10:

       |
    76 |   inherit version;
       |          ^
    
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/applications/networking/browsers/microsoft-edge/default.nix:102:3:

        |
    102 |   installPhase = ''
        |   ^
    

Comment on lines +116 to +119
test -e $out/share/microsoft/$appname/libEGL.so
ln -s libEGL.so $out/share/microsoft/$appname/libEGL.so.1
test -e $out/share/microsoft/$appname/libGLESv2.so
ln -s libGLESv2.so $out/share/microsoft/$appname/libGLESv2.so.2
Copy link
Contributor

Choose a reason for hiding this comment

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

are the test commands necessary anymore?

@@ -16846,6 +16846,8 @@ in

microsoft_gsl = callPackage ../development/libraries/microsoft_gsl { };

microsoft-edge-beta = callPackage ../applications/networking/browsers/microsoft-edge { gconf = gnome2.GConf; };
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @NixOS/gnome is there a better alternative to using something from gnome2?

Copy link
Contributor

@jtojnar jtojnar May 20, 2021

Choose a reason for hiding this comment

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

GConf should not be used at all these days, it has been deprecated for almost ten years.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both chromium and google-chrome have this dependency. It seems like upstream dropped GConf in favor of gsettings in 2017, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, almost nothing uses it these days. I started cleaning it up (#39976) but there are still many instances.

@leo60228
Copy link
Member Author

leo60228 commented May 20, 2021 via email

@SuperSandro2000
Copy link
Member

I based this heavily on the Google Chrome package. They're unsurprisingly very similar, I'd be surprised if anything was necessary on one but not the other.

That's because Edge is based on Chromium.

Comment on lines +67 to +71
libxkbcommon wayland
] ++ optional pulseSupport libpulseaudio
++ optional libvaSupport libva
++ optional vulkanSupport vulkan-loader
++ [ gtk3 ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
libxkbcommon wayland
] ++ optional pulseSupport libpulseaudio
++ optional libvaSupport libva
++ optional vulkanSupport vulkan-loader
++ [ gtk3 ];
libxkbcommon wayland gtk3
] ++ optional pulseSupport libpulseaudio
++ optional libvaSupport libva
++ optional vulkanSupport vulkan-loader;

Copy link
Member Author

Choose a reason for hiding this comment

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

Should this change also be applied to Chrome?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice. It might be even better to format the Chrome expression with nixpkgs-fmt, then do these cleanups and then fork it to edge.

@SeraphyBR
Copy link

?

@VergeDX
Copy link
Member

VergeDX commented Nov 18, 2021

I tested microsoft-edge-dev_94.0.982.2-1_amd64.deb and it works.
Why this PR not continue...? @leo60228

@zoedsoupe
Copy link

I also testes! LGTM. I think there're only some formatting issues

@totoroot
Copy link
Contributor

totoroot commented Apr 20, 2022

@leo60228 Can this PR be closed since #161064 got merged?

@leo60228 leo60228 closed this Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants