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

uqm: fix build #97919

Closed
wants to merge 1 commit into from
Closed

uqm: fix build #97919

wants to merge 1 commit into from

Conversation

liff
Copy link
Contributor

@liff liff commented Sep 13, 2020

Motivation for this change

ZHF: #97479

Things done
  1. The build failed with impurity error due to it wanting to write to /tmp. Replaced with $TMP.
  2. The game expects libpng to be available but the build doesn’t seem to link to it in any way. Added explicitly with patchelf.
  3. Enabled release build mode (debug build was default).
  • 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.

Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

  • Diff LGTM minus unnecessary patch file
  • Commits LGTM
  • Builds via nix-review
https://github.com/NixOS/nixpkgs/pull/97919
1 package built:
uqm

pkgs/games/uqm/default.nix Outdated Show resolved Hide resolved
@liff
Copy link
Contributor Author

liff commented Sep 15, 2020

Changed to use substituteInPlace, thanks.

Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

LGTM

https://github.com/NixOS/nixpkgs/pull/97919
1 package built:
uqm

@aanderse
Copy link
Member

Looks like @aszlig got around to fixing the build, though it appears they didn't quite test to find the libpng issue 😉. Do you mind rebasing this and sorting out what is required @liff? Thanks for your work so far!

@aszlig
Copy link
Member

aszlig commented Sep 19, 2020

@aanderse: Ah, thanks for pinging. You're correct that I didn't catch the libping issue since I'm usually just monitoring build failures and didn't do any tests at runtime.

@liff: What is the error you were getting about libpng? Using patchelf on something where the source code is available sounds wrong to me. Also, good catch on the debug build.

@aszlig
Copy link
Member

aszlig commented Sep 19, 2020

Hmm... apparently there are even new releases which also contain some fixes we could drop: 0.7.1, 0.7.2, 0.8.0
However, these are branches and not tags, so I'm not sure whether they officially count as "releases".

Update: Wrote an email asking for clarification, let's see :-)

@liff
Copy link
Contributor Author

liff commented Sep 19, 2020

This is the error without the libpng patch:

_GetCelData: Unable to load image!
Gfx Driver reports: Failed loading libpng16.so.16: libpng16.so.16: cannot open shared object file: No such file or directory

uqm uses SDL_image to load the images and apparently SDL_image locates the appropriate libraries dynamically. uqm build never refers to libpng directly.

Looks like adding -lpng to NIX_LDFLAGS works too.

@aszlig
Copy link
Member

aszlig commented Sep 19, 2020

uqm uses SDL_image to load the images and apparently SDL_image locates the appropriate libraries dynamically. uqm build never refers to libpng directly.

So if that's the case, then we ideally should fix SDL_image instead (Cc: @lovek323 who's the maintainer).

@jonringer
Copy link
Contributor

the tmp changes may no logner be necessary

@vcunat
Copy link
Member

vcunat commented Oct 18, 2020

Yes, the build succeeds on current master (and 20.09). I've done no testing; I didn't even know the package.

@SuperSandro2000
Copy link
Member

uqm uses SDL_image to load the images and apparently SDL_image locates the appropriate libraries dynamically. uqm build never refers to libpng directly.

So if that's the case, then we ideally should fix SDL_image instead (Cc: @lovek323 who's the maintainer).

@aszlig Can you do that instead of adding the missing dependency to here?

@aszlig
Copy link
Member

aszlig commented Nov 27, 2020

@aszlig Can you do that instead of adding the missing dependency to here?

What do you mean exactly? The issue has already been fixed in SDL_image (5813ab6).

@doronbehar
Copy link
Contributor

uqm builds and runs fine now.

@doronbehar doronbehar closed this Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants