Skip to content

build-fhsenv-bubblewrap: remove /usr/lib and /usr/lib32 from LD_LIBRARY_PATH#263201

Merged
Atemu merged 1 commit intoNixOS:masterfrom
LunNova:lunnova/build-fhs-env-no-usr-lib
Oct 25, 2023
Merged

build-fhsenv-bubblewrap: remove /usr/lib and /usr/lib32 from LD_LIBRARY_PATH#263201
Atemu merged 1 commit intoNixOS:masterfrom
LunNova:lunnova/build-fhs-env-no-usr-lib

Conversation

@LunNova
Copy link
Copy Markdown
Member

@LunNova LunNova commented Oct 24, 2023

Description of changes

#262775

#262080

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@LunNova LunNova requested review from Atemu, alyssais and wegank October 24, 2023 18:18
@LunNova
Copy link
Copy Markdown
Member Author

LunNova commented Oct 24, 2023

Result of nixpkgs-review pr 263201 run on x86_64-linux 1

1 package blacklisted:
  • appimage-run-tests
5 packages failed to build:
  • ciscoPacketTracer7
  • flexoptix-app
  • houdini
  • pureref
  • radicle-upstream
157 packages built:
  • Sylk
  • altair
  • android-studio
  • androidStudioPackages.beta
  • androidStudioPackages.canary
  • androidStudioPackages.dev
  • anki-bin
  • anytype
  • appimage-run
  • arduino
  • arduino-ci
  • arduino-cli
  • arduino-core
  • beekeeper-studio
  • beeper
  • betterdiscord-installer
  • beyond-identity
  • bitscope.chart
  • bitscope.console
  • bitscope.display
  • bitscope.dso
  • bitscope.logic
  • bitscope.meter
  • bitscope.proto
  • bitscope.server
  • bloomrpc
  • bootstrap-studio
  • bottles
  • burpsuite
  • buttercup-desktop
  • caprine-bin
  • chrysalis
  • cider
  • cockroachdb-bin
  • codux
  • conda
  • cozy-drive
  • crypto-org-wallet
  • davinci-resolve
  • devdocs-desktop
  • dropbox
  • dropbox-cli
  • dropbox-cli.nautilusExtension
  • electron-mail
  • electronplayer
  • esphome
  • esphome.dist
  • expressvpn
  • fahclient
  • firefly-desktop
  • fluent-reader
  • framesh
  • fspy
  • game-rs
  • hamsket
  • hifile
  • hover
  • immersed-vr
  • insync
  • irccloud
  • jbrowse
  • jetbrains-toolbox
  • joplin-desktop
  • keet
  • kingstvis
  • kodiPackages.steam-launcher
  • lbry
  • ldtk
  • ledger-live-desktop
  • left4gore-bin
  • lens
  • lightworks
  • localsend
  • losslesscut-bin
  • lunar-client
  • lunatask
  • lutris
  • lutris-free
  • marktext
  • mate.caja-dropbox
  • mathpix-snipping-tool
  • mendeley
  • minigalaxy
  • minigalaxy.dist
  • mobilecoin-wallet
  • mockoon
  • molotov
  • motrix
  • museeks
  • mycrypto
  • neo4j-desktop
  • nextflow
  • notable
  • notesnook
  • notion-app-enhanced
  • nuclear
  • onlyoffice-bin_7_4
  • openlens
  • osu-lazer-bin
  • p3x-onenote
  • pdfstudio2021
  • pdfstudio2022
  • pdfstudio2023
  • pdfstudioviewer
  • platformio
  • playonlinux
  • plex
  • plexamp
  • polypane
  • protontricks
  • protontricks.dist
  • protonup-qt
  • quartus-prime-lite
  • rambox
  • raven-reader
  • remnote
  • requestly
  • saleae-logic-2
  • session-desktop
  • shticker-book-unwritten
  • sidequest
  • simplex-chat-desktop
  • sonixd
  • space-station-14-launcher
  • sparrow
  • ssb-patchwork
  • station
  • steam
  • steam-rom-manager
  • steam-run
  • steam-small
  • steam-tui
  • steamPackages.steam-fhsenv-without-steam
  • steamPackages.steamcmd
  • teensyduino
  • timeular
  • trezor-suite
  • tusk
  • typora
  • uhk-agent
  • uhk-udev-rules
  • unityhub
  • unvanquished
  • upscayl
  • via
  • vial
  • vmware-workstation
  • vscode-fhs
  • vscodium-fhs
  • wootility
  • xivlauncher
  • xlights
  • ytmdesktop
  • zecwallet-lite
  • zettlr
  • zettlr-beta
  • zulip

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels Oct 24, 2023
Atemu
Atemu approved these changes Oct 25, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

We'll have to check whether this breaks the Steam runtime but otherwise, I don't see why we should have the lib dirs in LD_LIBRARY_PATH.

This was taken over from the old chroot made by @abbradar. Do you know why you added these in 3e395b7?

To get this merged quickly without testing steam, you could add these paths back in the Steam package (and verify LD_LIBRARY_PATH is still the same inside steam-run) and we can do that later.

@Atemu
Copy link
Copy Markdown
Member

Atemu commented Oct 25, 2023

Btw, have you ran a sample of dependant packages to check whether they still work? They should but you never know..

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Oct 25, 2023
@Atemu
Copy link
Copy Markdown
Member

Atemu commented Oct 25, 2023

Mangohud just stopped working but that happens regardless of your change and I doubt it'd cause that too.

@Atemu Atemu merged commit e2d06c5 into NixOS:master Oct 25, 2023
@github-actions github-actions bot mentioned this pull request Oct 25, 2023
Merged
1 task
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 25, 2023

Successfully created backport PR for release-23.05:

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 25, 2023

Git push to origin failed for release-23.05 with exitcode 1

@LunNova
Copy link
Copy Markdown
Member Author

LunNova commented Oct 25, 2023

I've been running it for a few days but hadn't tested steam. Quick check now and it still opens but haven't tested any games.

@nbdd0121 nbdd0121 mentioned this pull request Oct 26, 2023
Closed
@jshcmpbll jshcmpbll mentioned this pull request Nov 5, 2023
Open
@sbourdeauducq
Copy link
Copy Markdown
Contributor

sbourdeauducq commented Nov 18, 2023

If you build a FHS environment to run Xilinx Vivado, this patches breaks it as Vivado fails to find libtinfo (seems to be #218534).

@Atemu
Copy link
Copy Markdown
Member

Atemu commented Nov 18, 2023

@sbourdeauducq I'd recommend you to add the paths back for that particular package then.

@bjornfor
Copy link
Copy Markdown
Contributor

bjornfor commented Nov 20, 2023

Wasn't the premise for this PR that /usr/lib was in the search path already? Why do packages need fixes after this PR? (E.g. cdc306e.)

Can we fix this centrally (without relying on $LD_LIBRARY_PATH)? (I wonder what percentage of FHS env packages need fixes after this PR.)

@Atemu
Copy link
Copy Markdown
Member

Atemu commented Nov 20, 2023

If an FHS program requires default FHS library paths in LD_LIBRARY_PATH in order to run inside an FHS, it's quite honestly broken.

@bjornfor
Copy link
Copy Markdown
Contributor

bjornfor commented Nov 20, 2023

If an FHS program requires default FHS library paths in LD_LIBRARY_PATH in order to run inside an FHS, it's quite honestly broken.

Can you elaborate? I thought that on normal FHS distros /usr/lib was in the dynamic linker search path, so that you could do LD_PRELOAD=foo.so and it'd find /usr/lib/foo.so (if existing). That used to work in nixpkgs FHS envs too, but not since this PR. I don't see how the program being run inside the FHS can be at fault.

1 similar comment
@bjornfor
Copy link
Copy Markdown
Contributor

bjornfor commented Nov 20, 2023

If an FHS program requires default FHS library paths in LD_LIBRARY_PATH in order to run inside an FHS, it's quite honestly broken.

Can you elaborate? I thought that on normal FHS distros /usr/lib was in the dynamic linker search path, so that you could do LD_PRELOAD=foo.so and it'd find /usr/lib/foo.so (if existing). That used to work in nixpkgs FHS envs too, but not since this PR. I don't see how the program being run inside the FHS can be at fault.

@sbourdeauducq
Copy link
Copy Markdown
Contributor

sbourdeauducq commented Nov 21, 2023 •
edited
Loading

Not just FHS packages - it is nice to be able to spin FHS shells on NixOS to run whatever (possibly proprietary) binary apps that one needs without having to worry about Nix/Nixpkgs internals.

@LunNova
Copy link
Copy Markdown
Member Author

LunNova commented Nov 21, 2023 •
edited
Loading

LD_LIBRARY_PATH takes precedence over /nix/store deps in non-FHS executables, which was too high a priority and causes programs to run against unexpected library versions. That caused #262080 which reported widespread breakage inside FHS envs on the stable release.

I suggest raising new issues with specific breakages that you find since it's unlikely this will be reverted.

#89769 and #218534 track an issue with ncurses libtinfo.so's soname causing it not to be found by some FHS apps, which seems to be the main cause of apps relying on LD_LIBRARY_PATH being set.

@LunNova LunNova deleted the lunnova/build-fhs-env-no-usr-lib branch November 28, 2023 01:35
@LunNova LunNova mentioned this pull request Nov 28, 2023
Closed
13 tasks
pbsds pushed a commit that referenced this pull request Dec 2, 2023
@amarshall
davinci-resolve: fix runtime error
f1361c5 
See #265476

This is a hack that effectively undoes
#263201 for just this derivation.
Really we should probably be doing a ton of autoPatchelf or something to
patch in our libs. Alas, this is “good enough” to un-break for now, I
think.
@permahorse permahorse mentioned this pull request Dec 2, 2023
Open
permahorse added a commit to permahorse/houdini-nix that referenced this pull request Dec 4, 2023
@permahorse
a workaround for ncurses
30aba97 
workaround due to NixOS/nixpkgs#263201
explanation and taken from NixOS/nixpkgs#89769
@eamsden eamsden mentioned this pull request Dec 5, 2023
Merged
@delan delan mentioned this pull request Dec 9, 2023
Draft
14 tasks
github-actions bot pushed a commit that referenced this pull request Dec 11, 2023
@amarshall @github-actions
davinci-resolve: fix runtime error
9aae842 
See #265476

This is a hack that effectively undoes
#263201 for just this derivation.
Really we should probably be doing a ton of autoPatchelf or something to
patch in our libs. Alas, this is “good enough” to un-break for now, I
think.

(cherry picked from commit f1361c5)
joemfb added a commit to urbit/vere that referenced this pull request Mar 8, 2024
@joemfb
nix: update flake.lock to fix FHSUserEnv (#565)
f8bd512 
This updates nixpkgs in `flake.lock` to bring in a fix for the
`FHSUserEnv` required to run Bazel on NixOS. Without this fix, on recent
NixOS, an improper LD_LIBRARY_PATH will be set, causing glibc to crash
processes with "stack smashing detected."

See NixOS/nixpkgs#263201
eyJhb added a commit to eyJhb/robotnix that referenced this pull request Mar 31, 2024
@eyJhb
bumps nixpkgs to 23.11 and android-nixpkgs, remove nixpkgsUnstable
5159898 
This commit bumps nixpkgs to 23.11, while incorporating small changes
to ensure everything works.
At the same time, nixpkgsUnstable has been removed as it does not seem
to be needed anymore.
android-nixpkgs has been bumped to the latest version as well.

`LD_LIBRARY_PATH` paths has been added to buildFHSUSerEnv, because of
upstream changes. Please see the following information if curious.

NixOS/nixpkgs#103648
NixOS/nixpkgs#112266

NixOS/nixpkgs#262775
NixOS/nixpkgs#263201

NixOS/nixpkgs#278361
@eyJhb eyJhb mentioned this pull request Mar 31, 2024
Merged
Atemu pushed a commit to jaen/robotnix that referenced this pull request Jun 5, 2024
@eyJhb @Atemu
bumps nixpkgs to 23.11 and android-nixpkgs, remove nixpkgsUnstable
8dffd27 
This commit bumps nixpkgs to 23.11, while incorporating small changes
to ensure everything works.
At the same time, nixpkgsUnstable has been removed as it does not seem
to be needed anymore.
android-nixpkgs has been bumped to the latest version as well.

`LD_LIBRARY_PATH` paths has been added to buildFHSUSerEnv, because of
upstream changes. Please see the following information if curious.

NixOS/nixpkgs#103648
NixOS/nixpkgs#112266

NixOS/nixpkgs#262775
NixOS/nixpkgs#263201

NixOS/nixpkgs#278361
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Atemu Atemu Atemu approved these changes

@alyssais alyssais Awaiting requested review from alyssais

@wegank wegank Awaiting requested review from wegank

Assignees

No one assigned

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@LunNova @Atemu @sbourdeauducq @bjornfor @delroth