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.

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

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

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

LunNova
Copy link
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
Copy link
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

Copy link
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
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 reputable person label Oct 25, 2023
@Atemu
Copy link
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
21 checks passed
@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

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

@LunNova
Copy link
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.

@sbourdeauducq
Copy link
Contributor

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
Member

Atemu commented Nov 18, 2023

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

@bjornfor
Copy link
Contributor

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
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
Contributor

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
Contributor

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
Contributor

sbourdeauducq commented Nov 21, 2023

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
Member Author

LunNova commented Nov 21, 2023

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
pbsds pushed a commit that referenced this pull request Dec 2, 2023
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.
pedohorse added a commit to pedohorse/houdini-nix that referenced this pull request Dec 4, 2023
workaround due to NixOS/nixpkgs#263201
explanation and taken from NixOS/nixpkgs#89769
@delan delan mentioned this pull request Dec 9, 2023
14 tasks
github-actions bot pushed a commit that referenced this pull request Dec 11, 2023
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
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
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
Atemu pushed a commit to jaen/robotnix that referenced this pull request Jun 5, 2024
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
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 101-500 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants