Improve SDL setup hook

[sergv]

Summary

SDL setup hook does more work that required by putting more entries into SDL_LIB_PATH than needed. This contributes to bloated environment that leads to #41340.

Details

Before f7fdc99 the SDL setup hook looked like this (literally it was a bit different but logically SDL_LIB_PATH was modified only when SDL_PATH was too) :

addSDLPath () {
  if [ -e "$1/include/SDL" ]; then
    export SDL_PATH="${SDL_PATH-}${SDL_PATH:+ }$1/include/SDL"
    export SDL_LIB_PATH="${SDL_LIB_PATH-}${SDL_LIB_PATH:+ }-L$1/lib"
  fi
}

The last commit (f7fdc99) in the relevant PR #72812 changed the logic to:

addSDLPath () {
  if [ -e "$1/include/SDL" ]; then
    export SDL_PATH="${SDL_PATH-}${SDL_PATH:+ }$1/include/SDL"
  fi
  if [ -e "$1/lib" ]; then
    export SDL_LIB_PATH="${SDL_LIB_PATH-}${SDL_LIB_PATH:+ }-L$1/lib"
  fi
}

I was unable to find any justification for this change and the commit message looks suspicious: Merge commit 'afa48f16f265fd3e88073bca7929e1e103bd3dc3' into bash-no-undef-vars. The mentioned commit afa48f1 is not related to SDL so it seems like merge ought to have left SDL as is.

The way hook looks now, any package that has /lib gets added to SDL_LIB_PATH which is only used for SDL purposes within nixpkgs as far as I can tell. It seems that only SDL-related things should go there, like SDL_image, SDL_mixer, etc - basically the packages that have corresponding /include/SDL. Putting everything in there the way it works now is likely redundant.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.

[FliegendeWurst]

LGTM 👍

Since this fix causes a lot of rebuilds, please rebase the PR onto staging (howto).

[sergv]

Thanks for the review! I've rebased onto staging and changed merge target to staging as well.

[FliegendeWurst]

Per https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions the first line of the commit message should be: "SDL: put only SDL-related paths in SDL_LIB_PATH" (or similar).
It's very good that you included some context in the commit description!

[sergv]

Renamed the commit and rebased on staging once more.

[Ericson2314]

Looks good to me!

I think that was just a sloppy merge "huh, conditioning -L on include path? that looks like I made a mistake."

Good sleuthing, and thanks for catching my mistake.

[Artturin]

Probably won't work with a split dev though, I don't know a way around that.

[Ericson2314]

Oh huh good catch

[sergv]

Good point about packages with split dev, I didn't give them a thought before. However I'm probably missing something yet it seems they won't cause a problem: the SDL's setup hook initializes SDL_PATH and SDL_LIB_PATH variables which within nixpkgs are only consumed by the SDL itself (only mentioned in SDL's find-headers.patch).

As far as I understood from mentions of the environment variables the hook is intended to make packages like SDL_image, SDL_mixer, SDL_ttf or SDL_gfx found by the SDL. None of these packages have split dev. Only SDL itself uses split dev but it doesn't need to add itself to the environment variables sine it's the consumer of those variables.

[Artturin]

Add a comment that it's doesn't work with split dev because include and lib aren't in the same $1

[sergv]

I've added a note about split dev in the setup hook.

[Artturin]

A bot seems to have merged this with extra commits...

[Artturin]

@sergv Seems you rebased incorrectly and dropped your changes and github thought the pr was already merged :/ Can you make a new pr reconstructed from git reflog, I'll merge it.

Here's how you can rebase without dropping commits BTW https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#rebasing-between-branches-ie-from-master-to-staging

[sergv]

@Artturin looks like I may have accidentally removed branch in my repo, but I'm not fully sure. I've created new PR at #296015.