libgnome-keyring: Unify #321785
libgnome-keyring: Unify #321785
Conversation
Cherry-picked from 7570524
jtojnar
requested review from
sternenseemann,
maralorn and
ncfavier
as code owners
github-actions
bot
added
6.topic: GNOME
jtojnar
force-pushed
the
libgnome-keyring
branch
from
207846e to
1c20615
Compare
| libraryPkgconfigDepends = [ libgnome-keyring ]; | ||
| libraryToolDepends = [ c2hs ]; | ||
| description = "Bindings for libgnome-keyring"; | ||
| license = lib.licenses.gpl3Only; | ||
| badPlatforms = lib.platforms.darwin; | ||
| }) {inherit (pkgs.gnome) gnome-keyring; |
There was a problem hiding this comment.
Opened NixOS/cabal2nix#625 for that.
| # not ideal to use -config scripts but it's not possible switch it to pkg-config | ||
| # binaries in dev have a for build shebang |
There was a problem hiding this comment.
I don't understand the end of this sentence, but for sure this word is missing:
| # not ideal to use -config scripts but it's not possible switch it to pkg-config | |
| # binaries in dev have a for build shebang | |
| # not ideal to use -config scripts but it's not possible to switch it to pkg-config |
There was a problem hiding this comment.
Same here, I just cherry-picked @Artturin’s commit and gave up on it.
I believe it is supposed to be parsed “binaries in dev have (a (for build) shebang)”
| @@ -13,9 +13,23 @@ stdenv.mkDerivation (finalAttrs: { | |||
|
|
|||
| outputs = [ "out" "dev" ]; | |||
|
|
|||
| strictDeps = true; | |||
| propagatedBuildInputs = [ glib gobject-introspection dbus libgcrypt ]; | |||
| nativeBuildInputs = [ pkg-config intltool ]; | |||
There was a problem hiding this comment.
Maybe we should try to use autoreconfHook?
There was a problem hiding this comment.
I do not want to touch this. It should die already.
There was a problem hiding this comment.
I do not want to touch this. It should die already.
Oh I think the 1st commit's title is misleading - you are in fact editing the expression of the attribute libgnome-keyring, and not libgnome-keyring3.
There was a problem hiding this comment.
It is modifying what was gnome.libgnome-keyring (libgnome-keyring3) at the time of the commit. The package is only renamed later.
There was a problem hiding this comment.
I do not want to touch this. It should die already.
This is the comment that confused me BTW - why should it die?
There was a problem hiding this comment.
It adds extra maintenance work just to support few apps that could not be bothered to migrate to libsecret in the 10+ years libgnome-keyring has been deprecated.
There was a problem hiding this comment.
It adds extra maintenance work just to support few apps that could not be bothered to migrate to
libsecretin the 10+ years libgnome-keyring has been deprecated.
Ahh OK so libgnome-keyring is replaced by libgnome-keyring3 which should really be replaced by libsecret.
Still though, if we don't have that many packages still depending on libgnome-keyring or libgnome-keyring3, I'd make the effort to try to add autoreconfHook, but I don't care too much about this old library to insist on that :).
| @@ -22032,7 +22032,7 @@ with pkgs; | |||
|
|
|||
| libglibutil = callPackage ../development/libraries/libglibutil { }; | |||
|
|
|||
| libgnome-keyring = callPackage ../development/libraries/libgnome-keyring { }; | |||
| libgnome-keyring = gnome.libgnome-keyring; | |||
There was a problem hiding this comment.
Those libraries actually have the same API.
Hmm I tend to think maybe we should use a lib.warn alias instead? Maybe there are other differences we are not aware of?
There was a problem hiding this comment.
Well, one difference is that the newer version marks everything deprecated so it can cause problems if some project has deprecations disabled. But I do not expect that to happen – most consumers do not even include the headers, they just dlopen the library. And I would expect sizeable portion to be unused – the library has been deprecated for over 10 years.
Here is the abidiff for peace of mind:
$ nix shell nixpkgs#libabigail -c abidiff /nix/store/q36nxj41f071n2m7d3xffski2q02kvfa-libgnome-keyring-2.32.0/lib/libgnome-keyring.so.0.1.1 /nix/store/dyz4bhii86r5mqinayic77yv2rbfn8fr-libgnome-keyring-3.12.0/lib/libgnome-keyring.so.0.2.0
Functions changes summary: 0 Removed, 0 Changed, 0 Added function
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
Function symbols changes summary: 0 Removed, 12 Added function symbols not referenced by debug info
Variable symbols changes summary: 0 Removed, 1 Added variable symbol not referenced by debug info
12 Added function symbols not referenced by debug info:
[A] gnome_keyring_access_control_get_type
[A] gnome_keyring_application_ref_get_type
[A] gnome_keyring_attribute_get_string
[A] gnome_keyring_attribute_get_type
[A] gnome_keyring_attribute_get_uint32
[A] gnome_keyring_attribute_list_get_type
[A] gnome_keyring_attribute_list_new
[A] gnome_keyring_attribute_list_to_glist
[A] gnome_keyring_found_copy
[A] gnome_keyring_found_get_type
[A] gnome_keyring_info_get_type
[A] gnome_keyring_item_info_get_gtype
1 Added variable symbol not referenced by debug info:
[A] SECMEM_pool_data_v1_0There was a problem hiding this comment.
Well, one difference is that the newer version marks everything deprecated so it can cause problems if some project has deprecations disabled. But I do not expect that to happen – most consumers do not even include the headers, they just dlopen the library. And I would expect sizeable portion to be unused – the library has been deprecated for over 10 years.
Here is the abidiff for peace of mind:
OK Great it's nice to get confidence that this won't break anything, but still, the reason to add a lib.warn is to distribute the update - that's our role as a distribution IMO (same arguments as in the "How to introduce a breaking version bump" discourse thread).
There was a problem hiding this comment.
I do not think there is a consensus on lib.warn yet so I decided to go with status quo for aliases.nix and not add it here.
Those libraries actually have the same API version. The only difference is that the newer one just marks everything as deprecated.
jtojnar
force-pushed
the
libgnome-keyring
branch
from
1c20615 to
859183c
Compare
|
If the 1st commit has only a misleading title, but correct contents, it can be removed IMO. |
Not sure why you think it an be removed. My primary concern when deciding on commit contents is ease of reviewing and similar |
Yea I agree with this goal, I just got confused between the two version of the |
|
The guidelines require an existing attribute name as the commit header tag, so renames should be |
Description of changes
No need to have two API compatible copies of this ancient and deprecated library.
Things done
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.