adapta-gtk-theme: 3.94.0.132 -> 3.95.0.1 #47610

tadfisher · 2018-10-01T18:45:53Z

Motivation for this change

Update to the latest Adapta release.

Add override flags for all knobs that can be tweaked by Adapta's configuration scripts.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
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 nox --run "nox-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)
Fits CONTRIBUTING.md.

GrahamcOfBorg · 2018-10-01T19:05:27Z

Success on aarch64-linux (full log)

Attempted: adapta-gtk-theme

Partial log (click to expand)

make[3]: Leaving directory '/build/source'
make[2]: Leaving directory '/build/source'
make[1]: Leaving directory '/build/source'
glibPreFixupPhase
glibPreFixupPhase
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/zam6apa8xmx6f5h0zryihlmm0nb3x8ca-adapta-gtk-theme-3.95.0.1
strip is /nix/store/y4ymnvgxygpq05h03kyzbj572zmh6zla-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/zam6apa8xmx6f5h0zryihlmm0nb3x8ca-adapta-gtk-theme-3.95.0.1
checking for references to /build in /nix/store/zam6apa8xmx6f5h0zryihlmm0nb3x8ca-adapta-gtk-theme-3.95.0.1...

GrahamcOfBorg · 2018-10-01T19:09:05Z

Success on x86_64-linux (full log)

Attempted: adapta-gtk-theme

Partial log (click to expand)

make[3]: Leaving directory '/build/source'
make[2]: Leaving directory '/build/source'
make[1]: Leaving directory '/build/source'
glibPreFixupPhase
glibPreFixupPhase
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/xdzwc9ympxi93jwi0zg867sgs7b1ksdb-adapta-gtk-theme-3.95.0.1
strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/xdzwc9ympxi93jwi0zg867sgs7b1ksdb-adapta-gtk-theme-3.95.0.1
checking for references to /build in /nix/store/xdzwc9ympxi93jwi0zg867sgs7b1ksdb-adapta-gtk-theme-3.95.0.1...

timokau · 2018-10-01T19:31:09Z

pkgs/misc/themes/adapta/default.nix

+}:
+
+let
+  enableFlag = flag: en: stdenv.lib.optional en "--enable-${flag}";


Why not

enableFlag = flag: en: if en then "--enable-${flag}" else "--disable-"${flag}"

?

I'm pretty sure I've seen that pattern before, maybe its already in lib somewhere. Otherwise maybe it should be.

Ah, yes. I'll fix.

https://github.com/NixOS/nixpkgs/blob/master/lib/strings.nix#L424

timokau · 2018-10-01T19:31:57Z

pkgs/misc/themes/adapta/default.nix

-    "--disable-unity"
-  ];
+  configureFlags = [ "--enable-parallel" ]
+    ++ (disableFlag "gnome" enableGnome)


disableFlag returns a string, so why the ++?

It returns an array (using lib.optional).

But I'd rather use lib.enableFeature like you suggested.

timokau · 2018-10-01T19:33:09Z

pkgs/misc/themes/adapta/default.nix


-stdenv.mkDerivation rec {
+, # Enable Gnome Shell support


I think the comma should be in the same line as the flags. Most of these comments are pretty redundant anyways. I also don't much like the empty lines between all flags, but thats a matter of taste.

I can just remove these.

BTW, I based this off of existing arg documentation, such as fetchurl: https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/fetchurl/default.nix#L41

I agree that it looks odd.

Yeah there is a lot of weird formatting in the repo and since we don't have a comprehensive style guide I can only review based on my personal preference. I try not to block merging on that though.

We really should have a style guide, I just don't want to be the one that leads the bikeshed.

Some of the later comments are useful of course - just the enableFoo # enables foo ones could be removed.

timokau · 2018-10-01T20:41:45Z

Note that #47619 also does the same update.

tadfisher · 2018-10-01T20:55:12Z

@timokau Updated. lib does have a withFeatureAs, but these "--with-" flags are truly optional and don't have "--without-" counterparts, so I made a withOptional function.

tadfisher · 2018-10-01T20:56:31Z

@GrahamcOfBorg build adapta-gtk-theme

GrahamcOfBorg · 2018-10-01T21:00:18Z

Success on aarch64-linux (full log)

Attempted: adapta-gtk-theme

Partial log (click to expand)

make[3]: Leaving directory '/build/source'
make[2]: Leaving directory '/build/source'
make[1]: Leaving directory '/build/source'
glibPreFixupPhase
glibPreFixupPhase
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/gwlw9n47ya9blhylj116hdfsfccbf8px-adapta-gtk-theme-3.95.0.1
strip is /nix/store/y4ymnvgxygpq05h03kyzbj572zmh6zla-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/gwlw9n47ya9blhylj116hdfsfccbf8px-adapta-gtk-theme-3.95.0.1
checking for references to /build in /nix/store/gwlw9n47ya9blhylj116hdfsfccbf8px-adapta-gtk-theme-3.95.0.1...

GrahamcOfBorg · 2018-10-01T21:01:38Z

Success on x86_64-linux (full log)

Attempted: adapta-gtk-theme

Partial log (click to expand)

make[3]: Leaving directory '/build/source'
make[2]: Leaving directory '/build/source'
make[1]: Leaving directory '/build/source'
glibPreFixupPhase
glibPreFixupPhase
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/wnxrywqmc861w8gl7gh0qn9mmlm6z2vw-adapta-gtk-theme-3.95.0.1
strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/wnxrywqmc861w8gl7gh0qn9mmlm6z2vw-adapta-gtk-theme-3.95.0.1
checking for references to /build in /nix/store/wnxrywqmc861w8gl7gh0qn9mmlm6z2vw-adapta-gtk-theme-3.95.0.1...

GrahamcOfBorg · 2018-10-01T21:07:45Z

Success on aarch64-linux (full log)

Attempted: adapta-gtk-theme

Partial log (click to expand)

/nix/store/gwlw9n47ya9blhylj116hdfsfccbf8px-adapta-gtk-theme-3.95.0.1

GrahamcOfBorg · 2018-10-01T21:10:34Z

Success on x86_64-linux (full log)

Attempted: adapta-gtk-theme

Partial log (click to expand)

make[3]: Leaving directory '/build/source'
make[2]: Leaving directory '/build/source'
make[1]: Leaving directory '/build/source'
glibPreFixupPhase
glibPreFixupPhase
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/wnxrywqmc861w8gl7gh0qn9mmlm6z2vw-adapta-gtk-theme-3.95.0.1
strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/wnxrywqmc861w8gl7gh0qn9mmlm6z2vw-adapta-gtk-theme-3.95.0.1
checking for references to /build in /nix/store/wnxrywqmc861w8gl7gh0qn9mmlm6z2vw-adapta-gtk-theme-3.95.0.1...

timokau · 2018-10-01T21:20:03Z

pkgs/misc/themes/adapta/default.nix

+, enablePlank ? false
+, enableTelegram ? false
+, enableTweetdeck ? false
+, withSelectionColor ? null   # Primary color for 'selected-items' (Default: #3F51B5 = Indigo500)


Please don't align the comments

timokau · 2018-10-01T21:20:44Z

pkgs/misc/themes/adapta/default.nix

+  configureFlags =
+    let
+      inherit (stdenv.lib) enableFeature optional;
+      withOptional = feat: value: optional (value != null) "--with-${feat}=${value}";


Are you sure there is no --without counterpart? Isn't that an autotools standard? Or are they not using autotools?

They are using autotools, but these options are provided via custom m4 scripts. These are simple customization options, not feature flags.

timokau · 2018-10-01T21:22:29Z

pkgs/misc/themes/adapta/default.nix

+      withOptional = feat: value: optional (value != null) "--with-${feat}=${value}";
+    in [
+      "--enable-parallel"
+      (enableFeature enableGnome     "gnome")


I'm confused: Wouldnt't this result in --enable-gnome=true (does nix coalesce booleans to strings)? Shouldn't it be = yes?

Also please no alignment here.

enableFeature doesn't produce assignments, so you'll get either --enable-{feature} or --disable-{feature}.

tadfisher · 2018-10-03T17:08:26Z

@timokau Removed code alignment.

tadfisher · 2018-10-03T17:08:38Z

@GrahamcOfBorg build adapta-gtk-theme

GrahamcOfBorg · 2018-10-03T17:12:33Z

Success on aarch64-linux (full log)

Attempted: adapta-gtk-theme

Partial log (click to expand)

make[3]: Leaving directory '/build/source'
make[2]: Leaving directory '/build/source'
make[1]: Leaving directory '/build/source'
glibPreFixupPhase
glibPreFixupPhase
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/9dlwflh2zf2q1yf5zday5xjn70bg6ndc-adapta-gtk-theme-3.95.0.1
strip is /nix/store/428gs2z4b8f9byvghzlpbjwjb3a7jwxx-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/9dlwflh2zf2q1yf5zday5xjn70bg6ndc-adapta-gtk-theme-3.95.0.1
checking for references to /build in /nix/store/9dlwflh2zf2q1yf5zday5xjn70bg6ndc-adapta-gtk-theme-3.95.0.1...

GrahamcOfBorg · 2018-10-03T17:12:58Z

Success on x86_64-linux (full log)

Attempted: adapta-gtk-theme

Partial log (click to expand)

make[3]: Leaving directory '/build/source'
make[2]: Leaving directory '/build/source'
make[1]: Leaving directory '/build/source'
glibPreFixupPhase
glibPreFixupPhase
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/p03r415ig2gvhvfhxzfc20ivy3qh2f6f-adapta-gtk-theme-3.95.0.1
strip is /nix/store/dxf1m7dhc4qb655bdljc1fsd74v1nag3-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/p03r415ig2gvhvfhxzfc20ivy3qh2f6f-adapta-gtk-theme-3.95.0.1
checking for references to /build in /nix/store/p03r415ig2gvhvfhxzfc20ivy3qh2f6f-adapta-gtk-theme-3.95.0.1...

tadfisher · 2018-10-03T17:13:21Z

@timokau Upstream has reverted to the previous version, so this PR is no longer valid. Closing.

GrahamcOfBorg · 2018-10-03T17:24:47Z

Success on aarch64-linux (full log)

Attempted: adapta-gtk-theme

Partial log (click to expand)

/nix/store/9dlwflh2zf2q1yf5zday5xjn70bg6ndc-adapta-gtk-theme-3.95.0.1

GrahamcOfBorg · 2018-10-03T17:32:16Z

Success on x86_64-linux (full log)

Attempted: adapta-gtk-theme

Partial log (click to expand)

make[3]: Leaving directory '/build/source'
make[2]: Leaving directory '/build/source'
make[1]: Leaving directory '/build/source'
glibPreFixupPhase
glibPreFixupPhase
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/p03r415ig2gvhvfhxzfc20ivy3qh2f6f-adapta-gtk-theme-3.95.0.1
strip is /nix/store/dxf1m7dhc4qb655bdljc1fsd74v1nag3-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/p03r415ig2gvhvfhxzfc20ivy3qh2f6f-adapta-gtk-theme-3.95.0.1
checking for references to /build in /nix/store/p03r415ig2gvhvfhxzfc20ivy3qh2f6f-adapta-gtk-theme-3.95.0.1...

timokau · 2018-10-03T18:01:46Z

They removed a release? Very weird.

tadfisher force-pushed the adapta branch from df6336a to 714013e Compare October 1, 2018 18:49

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Oct 1, 2018

timokau reviewed Oct 1, 2018

View reviewed changes

dtzWill mentioned this pull request Oct 1, 2018

adapta-gtk-theme: 3.94.0.149 -> 3.95.0.1 #47619

Closed

9 tasks

tadfisher force-pushed the adapta branch from 714013e to 563ef51 Compare October 1, 2018 20:53

timokau reviewed Oct 1, 2018

View reviewed changes

timokau requested changes Oct 1, 2018

View reviewed changes

adapta-gtk-theme: 3.94.0.132 -> 3.95.0.1

44fd8cc

tadfisher force-pushed the adapta branch from 563ef51 to 44fd8cc Compare October 3, 2018 17:08

tadfisher closed this Oct 3, 2018

tadfisher mentioned this pull request Dec 17, 2018

plata-theme: init at 0.4.9 #52425

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adapta-gtk-theme: 3.94.0.132 -> 3.95.0.1 #47610

adapta-gtk-theme: 3.94.0.132 -> 3.95.0.1 #47610

tadfisher commented Oct 1, 2018

GrahamcOfBorg commented Oct 1, 2018

GrahamcOfBorg commented Oct 1, 2018

timokau Oct 1, 2018

tadfisher Oct 1, 2018

timokau Oct 1, 2018

tadfisher Oct 1, 2018

timokau Oct 1, 2018

tadfisher Oct 1, 2018

tadfisher Oct 1, 2018

timokau Oct 1, 2018

timokau Oct 1, 2018 •

edited

timokau commented Oct 1, 2018

tadfisher commented Oct 1, 2018

tadfisher commented Oct 1, 2018

GrahamcOfBorg commented Oct 1, 2018

GrahamcOfBorg commented Oct 1, 2018

GrahamcOfBorg commented Oct 1, 2018

GrahamcOfBorg commented Oct 1, 2018

timokau Oct 1, 2018

timokau Oct 1, 2018

tadfisher Oct 3, 2018

timokau Oct 1, 2018

tadfisher Oct 3, 2018

tadfisher commented Oct 3, 2018

tadfisher commented Oct 3, 2018

GrahamcOfBorg commented Oct 3, 2018

GrahamcOfBorg commented Oct 3, 2018

tadfisher commented Oct 3, 2018

GrahamcOfBorg commented Oct 3, 2018

GrahamcOfBorg commented Oct 3, 2018

timokau commented Oct 3, 2018

adapta-gtk-theme: 3.94.0.132 -> 3.95.0.1 #47610

adapta-gtk-theme: 3.94.0.132 -> 3.95.0.1 #47610

Conversation

tadfisher commented Oct 1, 2018

Motivation for this change

Things done

GrahamcOfBorg commented Oct 1, 2018

GrahamcOfBorg commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timokau Oct 1, 2018 • edited

Choose a reason for hiding this comment

timokau commented Oct 1, 2018

tadfisher commented Oct 1, 2018

tadfisher commented Oct 1, 2018

GrahamcOfBorg commented Oct 1, 2018

GrahamcOfBorg commented Oct 1, 2018

GrahamcOfBorg commented Oct 1, 2018

GrahamcOfBorg commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadfisher commented Oct 3, 2018

tadfisher commented Oct 3, 2018

GrahamcOfBorg commented Oct 3, 2018

GrahamcOfBorg commented Oct 3, 2018

tadfisher commented Oct 3, 2018

GrahamcOfBorg commented Oct 3, 2018

GrahamcOfBorg commented Oct 3, 2018

timokau commented Oct 3, 2018

timokau Oct 1, 2018 •

edited