Conversation
|
I'm a bit confused with this change, I'll have a look when I'm at my PC. |
What confuses you? |
There was a problem hiding this comment.
I'd rather not move back to the rec could you keep the finalAttrs
There was a problem hiding this comment.
I'm not sure you should just remove all these people. They all clearly have interest in the package and are active.
See:
There was a problem hiding this comment.
Hmm, then it is interesting why after update to 25.05 I got message that the package is unmaintained 🤔
There was a problem hiding this comment.
The package was briefly removed due to there only being one maintainer. Then all these lovely people stepped up :D
There was a problem hiding this comment.
Thank you from not removing us from the maintainers list 🙂
There was a problem hiding this comment.
could we at least replace the update script :p
|
The package is not unmaintained. The current maintainers were added after the package was removed for lack of maintenance recently in the commits that Isabel linked above Please feel free to add yourself as a new maintainer though! |
| Soveu = { | ||
| name = "Soveu"; | ||
| github = "Soveu"; | ||
| githubId = 34382234; | ||
| }; |
There was a problem hiding this comment.
This should be in its own commit before the commit altering microsoft-edge. The commit should be called maintainers: add Soveu see https://github.com/NixOS/nixpkgs/blob/master/maintainers/README.md#how-to-become-a-maintainer for more detail.
oh, then I'm late to the party 😅 Sorry for the confusion |
|
you can just change the PR to instead update to |
| in | ||
|
|
||
| stdenvNoCC.mkDerivation (finalAttrs: { | ||
| stdenv.mkDerivation rec { |
There was a problem hiding this comment.
Also should note that this was specifically moved to stdenvNoCC (see #415521) is there a reason for moving it back?
There was a problem hiding this comment.
I think he started more or less from a blank file and was not aware we already took over maintaining the package
There was a problem hiding this comment.
dependency is needed for stdenv.cc.cc in buildDependencies
|
And since we aren't the original code owners, maybe it does make sense to use autopatchelf instead, but for that I want to first understand why it was done this way deliberately |
There is a lot more going on for this package - basically I assumed it was still unmaintained and started from scratch, because the original derivation was... very convoluted to say the least |
Yeah there is a lot going on, but since presumably most of it was done deliberately I would be careful before just changing it |
| url = "mirror://gnome/sources/libxml2/${lib.versions.majorMinor version}/libxml2-${version}.tar.xz"; | ||
| hash = "sha256-J3KUyzMRmrcbK8gfL0Rem8lDW4k60VuyzSsOhZoO6Eo="; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Going out of our way to use a known vulnerable libxml2 version in a web browser does not seem like a good idea to me.
There was a problem hiding this comment.
Agreeing on behalf of @NixOS/security.
@ulrikstrid Please review and respond to this PR.
Hey, where did you see that |
| mv "$icon_file" "$logo_output_path/microsoft-edge.png" | ||
| done | ||
|
|
||
| # "--simulate-outdated-no-au" disables auto updates and browser outdated popup |
There was a problem hiding this comment.
We should not leave this out, since otherwise you keep get annoying popups
|
could we just reject this PR and then just open a PR to refactor potentially convoluted parts without using the libxml workaround? |
Hmm, while looking for newest version I just look directly inside https://packages.microsoft.com/repos/edge/pool/main/m/microsoft-edge-stable/ |
Yeah |
|
On my side, I'm not sure about this refactor. A lot of dependencies seems to be removed (like all the qt plugins or all the wayland/ozone packages) without really knowing the consequences afterward. (Please explain me if I missed something) |
|
I will go back to work... my phone started blowing up, @Soveu if you want to, you could open an Issue, then we can discuss potential refactor steps there |
|
i suggest we do open 2 PRs :
|
Sure, no pressure 😉
It was not needed to make it work and as far as I know, wayland/ozone is only necessary for nvidia users? And even then I'm not sure if they really need it on newer GPU drivers. |
Hum, no ? Wayland/Ozone isn't tied to nvidia, it is there to actually run the browser in DE using wayland (like gnome or KDE). |
As this package from 25.05 is unmaintained, I can step up to be responsible for it.
Basically, I started from scratch, it turned out to be relatively easy.
Edge also required the
libxml2workaround, see those:Testing locally, I was able to successfully sync Edge with my company account and also I don't see any major errors in logs.
Things done
Updated version to
138.0.3351.55-1, useautoPatchelfHookandqt6.wrapQtAppsHook, remove the packaged QT5 shim.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.