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.

Sign up for GitHub

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

Jump to bottom

gnomeExtensions: Some more cleanup #124315

Merged
merged 16 commits into from
Jul 14, 2021
Merged

gnomeExtensions: Some more cleanup #124315

merged 16 commits into from
Jul 14, 2021

Conversation

piegamesde
Copy link
Member

@piegamesde piegamesde commented May 24, 2021 •
edited
Loading

Motivation for this change
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/)
  • Added a release notes entry if the change is major or breaking
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: GNOME GNOME desktop environment and its underlying platform label May 24, 2021
@piegamesde piegamesde requested a review from jtojnar May 24, 2021 23:08
@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package labels May 24, 2021
@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 May 24, 2021
@jtojnar

This comment has been minimized.

Sign in to view
jtojnar
jtojnar reviewed May 24, 2021
View reviewed changes
pkgs/desktops/gnome/extensions/draw-on-you-screen/default.nix Outdated
@@ -1,7 +1,7 @@
{ lib, stdenv, fetchgit, gettext, gnome }:

stdenv.mkDerivation rec {
pname = "gnome-shell-extension-draw-on-your-screen";
pname = "gnome-shell-extension-draw-on-you-screen";
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a typo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it very probably is. Nevertheless, this is the "official" name, and thus the name of the automatically packaged extension ¯_(ツ)_/¯

Copy link
Member

Choose a reason for hiding this comment

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

I would not consider that an official name – it is not used anywhere other than the slug on the extension portal website AFAICT.

@piegamesde
Copy link
Member Author

Regarding passthru, I just recently realized that the manually packaged extensions simply expose uuid in the attrset and people may depend on this. I don't care how we do it out of these two options, but I'd like it to be consistent in some way.

jtojnar
jtojnar reviewed May 24, 2021
View reviewed changes
pkgs/desktops/gnome/extensions/no-title-bar-forked/default.nix Outdated
@@ -1,7 +1,7 @@
{ lib, stdenv, fetchFromGitHub, substituteAll, glib, gettext, xorg }:

stdenv.mkDerivation rec {
pname = "gnome-shell-extension-no-title-bar";
pname = "gnome-shell-extension-no-title-bar-forked";
Copy link
Member

Choose a reason for hiding this comment

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

This also sounds like something we should not need.

Copy link
Member Author

Choose a reason for hiding this comment

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

So my point is, the manually packaged extensions must somehow match the automatically packaged ones in order for overriding to work.

Copy link
Member

Choose a reason for hiding this comment

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

Could we do it in the other direction – rename the automatically packaged extension instead? Since the names are arbitrary, we might as well use ones that make sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Middle ground: I'd like to keep the pname the same, but I'd be fine with renaming the attribute name of the extensions (pname is almost worthless anyways).

The most promising approach at implementing this would be to key all manually packaged extensions with their UUID as well, so that I can first do the merge/override, and then give them proper names.

Copy link
Member

Choose a reason for hiding this comment

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

pname is what Repology uses so we should keep it somewhat canonical and reasonable.

I would say the easiest would be adding a section for changing bad slugs in extensionRenames.nix. But adding detection of duplicates based on UUID would be useful when people add a new manually packaged extension so that they choose a correct name.

@jtojnar
Copy link
Member

jtojnar commented May 25, 2021

Pushed the fix for the accidental rename as 571d540.

@ofborg ofborg bot added the ofborg-internal-error Ofborg encountered an error label May 29, 2021
@piegamesde
Copy link
Member Author

Second try, I almost started from scratch. No renames just yet, as they are less important with the improved merging. Personally, I'd like to rename all folders of the manually packaged extensions to use their UUID instead of their pname, but let me know what you think about this first.

@piegamesde
Copy link
Member Author

So I went ahead and removed all manual packages that looked like the usual generic boilerplate. @chkno @jtojnar @emanueleperuffo @svsdep @hedning @tu-maurice @benley @Mdsp9070 @ericdallo @alesguzik @eduardosm @jonafato your extensions are affected. Please have a look and comment if any of them can not simply be replaced by the automatic packaging.

@piegamesde piegamesde changed the title gnomeExtensions: Normalize pnames even more gnomeExtensions: Some more cleanup May 29, 2021
@piegamesde piegamesde force-pushed the gnome-extensions branch 5 times, most recently from 139aeab to 0071880 Compare May 30, 2021 00:06
@piegamesde piegamesde force-pushed the gnome-extensions branch 2 times, most recently from 1c4c1bd to 1565c24 Compare July 14, 2021 00:48
@piegamesde
Copy link
Member Author

piegamesde commented Jul 14, 2021 •
edited by jtojnar
Loading

Let's summarize the discussion so far because the next time I'll have forgotten most of it:

  • We should take a look a the pnames and compare with Repology
  • The extensionPortalSlug can be normalized some more.
  • Evaluate if some manual renames are worse than the extension portal slugs and drop them (adding legacy alias).
  • (list will be expanded)

@piegamesde piegamesde force-pushed the gnome-extensions branch 2 times, most recently from 90c868b to 37d6219 Compare July 14, 2021 01:01
piegamesde added 14 commits July 14, 2021 03:35
@piegamesde
gnomeExtensions: integrate manually packaged extensions better with t…
bffbf46 
…he others

Their keys are now not used directly. Instead, we go the standard route of mapping to the
UUID in order to apply the rename procedure. This makes sure the manual override always does
the correct thing, and also gives us more consistency overall.
@piegamesde
gnomeExtensions.workspace-matrix: Remove manual packaging
5128ec5
@piegamesde
gnomeExtensions.window-is-ready-remover: Remove manual packaging
ba3562e
@piegamesde
gnomeExtensions.timepp: Remove manual packaging
c5aa58a
@piegamesde
gnomeExtensions.noannoyance: Remove manual packaging
33cf653
@piegamesde
gnomeExtensions.mpris-indicator-button: Remove manual packaging
ee5b47e
@piegamesde
gnomeExtensions.material-shell: Remove manual packaging
9dcb9ae
@piegamesde
gnomeExtensions.hot-edge: Remove manual packaging
8dd6024
@piegamesde
gnomeExtensions: Remove gtile
a4d668b 
It was never actually referenced anywhere, so technically it was never packaged …
@piegamesde
gnomeExtensions.draw-on-your-screen: Remove manual packaging
2e0ffbe
@piegamesde
gnomeExtensions.disable-unredirect: Remove manual packaging
1149365
@piegamesde
gnomeExtensions.clipboard-indicator: Remove manual packaging
1d1a498
@piegamesde
gnomeExtensions.dynamic-panel-transparency: Remove manual packaging
e7d7df7
@piegamesde
gnomeExtensions.appindicator: Remove manual packaging
3c85c0c
jtojnar
jtojnar approved these changes Jul 14, 2021
View reviewed changes
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for bearing with me.

@jtojnar jtojnar merged commit 5fb893f into NixOS:master Jul 14, 2021
@piegamesde
Copy link
Member Author

Thanks for all the reviews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eduardosm eduardosm eduardosm left review comments

@jtojnar jtojnar jtojnar approved these changes

@emanueleperuffo emanueleperuffo emanueleperuffo approved these changes

@jonafato jonafato Awaiting requested review from jonafato

@alesya-h alesya-h Awaiting requested review from alesya-h

@amaxine amaxine Awaiting requested review from amaxine

@rhoriguchi rhoriguchi Awaiting requested review from rhoriguchi

@doronbehar doronbehar Awaiting requested review from doronbehar

@ericdallo ericdallo Awaiting requested review from ericdallo

Assignees
No one assigned
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@piegamesde @jtojnar @rhoriguchi @chkno @wucke13 @eduardosm @emanueleperuffo @cole-h