Skip to content

Remove thin provisioning tools dependency from lvm2#121389

Merged
FRidh merged 1 commit intoNixOS:stagingfrom
arianvp:remove-thin-provisioning-tools
Jul 29, 2021
Merged

Remove thin provisioning tools dependency from lvm2#121389
FRidh merged 1 commit intoNixOS:stagingfrom
arianvp:remove-thin-provisioning-tools

Conversation

@arianvp
Copy link
Member

@arianvp arianvp commented May 1, 2021

thin-provisioning-tools has a huge closure size (hundreds of
megabytes) and the only reference in the output of the lvm2 package is a
comment in the etc/lvm.conf

The lvm2 package thus does not seem to depend on thin-provisoning-tools
in any way.

Reverts 9326a89

thin provisoning is broken with or without this change:
#15516

People were already doing this revert locally it seems:
#14394

A proper fix is here:
#46541

References:

$ nix why-depends nixpkgs#lvm2 nixpkgs#thin-provisioning-tools
/nix/store/n7zwwxi0ihjks7qk9bq5lbkniligfcqc-lvm2-2.03.11
└───etc/lvm.conf: …_check_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-prov>
→ /nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0

$ ag thin-provisioning-tools --search-binary
etc/lvm.conf
1093: # (See package device-mapper-persistent-data or thin-provisioning-tools)
1095: # thin_check_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/thin_check"
1100: # (See package device-mapper-persistent-data or thin-provisioning-tools)
1102: # thin_dump_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/thin_dump"
1108: # (See package device-mapper-persistent-data or thin-provisioning-tools)
1110: # thin_repair_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/thin_repair"
1155: # (See package device-mapper-persistent-data or thin-provisioning-tools)
1157: # cache_check_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/cache_check"
1162: # (See package device-mapper-persistent-data or thin-provisioning-tools)
1164: # cache_dump_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/cache_dump"
1170: # (See package device-mapper-persistent-data or thin-provisioning-tools)
1172: # cache_repair_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/cache_repair"

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/)
  • 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.

thin-provisioning-tools has a _huge_ closure size (hundreds of
megabytes) and the only reference in the output of the lvm2 package is a
_comment_ in the etc/lvm.conf

The lvm2 package thus does not seem to depend on thin-provisoning-tools
in any way.

Reverts 9326a89

thin provisoning is broken with or without this change:
NixOS#15516

A proper fix is here:
NixOS#46541

References:

$ nix why-depends nixpkgs#lvm2 nixpkgs#thin-provisioning-tools
/nix/store/n7zwwxi0ihjks7qk9bq5lbkniligfcqc-lvm2-2.03.11
└───etc/lvm.conf: …_check_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-prov>
    → /nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0

$ ag thin-provisioning-tools --search-binary
etc/lvm.conf
1093:	# (See package device-mapper-persistent-data or thin-provisioning-tools)
1095:	# thin_check_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/thin_check"
1100:	# (See package device-mapper-persistent-data or thin-provisioning-tools)
1102:	# thin_dump_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/thin_dump"
1108:	# (See package device-mapper-persistent-data or thin-provisioning-tools)
1110:	# thin_repair_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/thin_repair"
1155:	# (See package device-mapper-persistent-data or thin-provisioning-tools)
1157:	# cache_check_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/cache_check"
1162:	# (See package device-mapper-persistent-data or thin-provisioning-tools)
1164:	# cache_dump_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/cache_dump"
1170:	# (See package device-mapper-persistent-data or thin-provisioning-tools)
1172:	# cache_repair_executable = "/nix/store/w5an38q6byfr1sihks66srbqdii9hnsd-thin-provisioning-tools-0.9.0/bin/cache_repair"
@arianvp arianvp requested a review from globin May 1, 2021 11:45
@ofborg ofborg bot requested review from 7c6f434c and ajs124 May 1, 2021 11:57
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels May 1, 2021
Copy link
Member

@7c6f434c 7c6f434c left a comment

Choose a reason for hiding this comment

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

Makes sense

@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 121389 at 5e7c66c run on x86_64-linux 1

736 packages marked as broken and skipped:
  • adobe-reader
  • adoptopenjdk-hotspot-bin-13
  • adoptopenjdk-hotspot-bin-14
  • adoptopenjdk-jre-hotspot-bin-13
  • adoptopenjdk-jre-hotspot-bin-14
  • adoptopenjdk-jre-openj9-bin-13
  • adoptopenjdk-jre-openj9-bin-14
  • adoptopenjdk-openj9-bin-13
  • adoptopenjdk-openj9-bin-14
  • agdaPackages.iowa-stdlib
  • ...
1 package failed to build:
8401 packages skipped due to time constraints:
  • AusweisApp2
  • CHOWTapeModel
  • DisnixWebService
  • EBTKS
  • EmptyEpsilon
  • MIDIVisualizer
  • MMA
  • OSCAR
  • OVMF
  • OVMF-CSM
  • ...
42 packages built successfully:
  • SDL
  • SDL2
  • SDL2_image
  • openjdk8-bootstrap (adoptopenjdk-hotspot-bin-8)
  • at-spi2-atk
  • at-spi2-core
  • avahi
  • cryptsetup
  • cups
  • dbus
  • dbus-glib
  • dconf (gnome3.dconf ,xfce.dconf)
  • dmraid
  • dnsmasq
  • ffmpeg (ffmpeg_4)
  • gtk3 (gnome3.gtk ,gtk3-x11)
  • libgudev (gnome3.libgudev)
  • libsecret (gnome3.libsecret)
  • tracker (gnome3.tracker)
  • libatasmart
  • libdbusmenu-gtk3
  • libinput
  • libjack2
  • libmbim
  • libpulseaudio (libpulseaudio-vanilla)
  • libwacom
  • lvm2
  • lvm2_dmeventd
  • mdadm (mdadm4)
  • media-player-info
  • mount (unixtools.mount)
  • openconnect (openconnect_gnutls)
  • pmutils
  • postgresql (postgresql_11)
  • procps
  • python38Packages.dbus-python
  • python38Packages.pyudev
  • stoken
  • udev (systemd)
  • umount (unixtools.umount)
  • util-linux (util-linuxCurses)
  • wrapGAppsNoGuiHook
6 suggestions:
  • warning: unclear-gpl

    gpl2 is a deprecated license, please check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

    Near pkgs/os-specific/linux/lvm2/default.nix:125:5:

        |
    125 |     license = with licenses; [ gpl2 bsd2 lgpl21 ];
        |     ^
    
  • warning: unclear-gpl

    lgpl21 is a deprecated license, please check if project uses lgpl21Plus or lgpl21Only and change meta.license accordingly.

    Near pkgs/os-specific/linux/lvm2/default.nix:125:5:

        |
    125 |     license = with licenses; [ gpl2 bsd2 lgpl21 ];
        |     ^
    
  • warning: missing-patch-comment

    Please add a comment on the line above, explaining the purpose of this patch.
    Near pkgs/os-specific/linux/lvm2/default.nix:72:5:

       |
    72 |     (fetchpatch {
       |     ^
    
  • warning: unused-argument

    Unused argument: util-linux.
    Near pkgs/os-specific/linux/lvm2/default.nix:5:3:

      |
    5 | , util-linux
      |   ^
    
  • warning: missing-patch-comment

    Please add a comment on the line above, explaining the purpose of this patch.
    Near pkgs/os-specific/linux/lvm2/default.nix:82:5:

       |
    82 |     (fetchpatch {
       |     ^
    
  • warning: missing-patch-comment

    Please add a comment on the line above, explaining the purpose of this patch.
    Near pkgs/os-specific/linux/lvm2/default.nix:77:5:

       |
    77 |     (fetchpatch {
       |     ^
    

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement.

@ajs124
Copy link
Member

ajs124 commented May 1, 2021

A proper fix is here:
#46541

AFAIR that has basically hit master through #93024. This PR doesn't look like it'll break anything, but

thin provisoning is broken with or without this change:

is objectively wrong, because I run literally dozens of NixOS systems from lvm thin volumes.

EDIT: https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/tasks/lvm.nix#L44 for reference.

@arianvp
Copy link
Member Author

arianvp commented May 1, 2021

Thanks for the links @ajs124 . I suggest I adjust the commit message to reflect your remarks.

@ajs124
Copy link
Member

ajs124 commented May 1, 2021

@arianvp Sounds like a plan 👍

Someone (maybe me) should probably also go through the things you linked and see which ones of them are still relevant. I don't remember why I didn't close them back then.

@FRidh FRidh merged commit d5fde7f into NixOS:staging Jul 29, 2021
@ajs124
Copy link
Member

ajs124 commented Jul 29, 2021

Would have been nice to have the adjusted commit message, because as is, it might mislead people.

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

Labels

10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants