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

unigine-valley: refactor #21202

Merged
merged 4 commits into from
Jan 11, 2017
Merged

unigine-valley: refactor #21202

merged 4 commits into from
Jan 11, 2017

Conversation

kierdavis
Copy link
Contributor

Motivation for this change

I was quite inexperienced with Nix/NixOS when I wrote this expression a couple of months ago. This PR cleans up the expression to be more consistent with other Nix expressions.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Changes:

  • Remove unnecessary and un-Nix-y "release version".
  • Only run strip over the directory containing binaries and shared libraries, rather than the entire installation directory.
  • Change install location from $out/opt/unigine/valley to $out/lib/unigine/valley.
  • Define the above install location in a derivation variable, so that it doesn't have to copy-pasted everywhere.
  • Remove debug messages from ELF patching phase.
  • Clean up some comments.

There are a few other things that I'd like to fix in the near future, but need some more input:

  • The upstream package should work on 32-bit and 64-bit x86 systems; however I think this expression will only work on 64-bit systems, since I've hardcoded the ELF interpreter path to ${stdenv.cc.libc}/lib/ld-linux-x86-64.so.2. Do I need to use a different ELF interpreter for 32-bit ELFs? If so, what path can it be found at? Alternatively, is choosing an appropriate ELF interpreter already handled somewhere else in nixpkgs, so I can just use something along the lines of ${stdenv.elfInterpreter} instead?
  • This expression only works on NixOS at the moment. Getting it to work on Linux+Nix or macOS+Nix systems would require locating libGL.so.1 on that system. Is there a built in way to pick the right directory to add to LD_LIBRARY_PATH for this, or does this expression need to do the logic itself?
  • As requested in [Package request] unigine #20547, I think it should be straightforward to package Unigine's other graphics benchmarks. Perhaps a single generic expression could be used for all of them.

The upstream version is "1.0", so that's what the version of the Nix package should be too.
When I packaged this I wasn't aware that a Nix package could update without its version number
increasing, so I added an extra "release version" (like Arch Linux packages). Of course, this
isn't necessary.
Previously, the entire installation was copied to $out/opt/unigine/valley.
Using $out/lib instead of $out/opt would be more consistent with other Nix packages.
@kierdavis
Copy link
Contributor Author

CI failure seems to be unrelated. I'll try again when things are greener.

@joachifm joachifm merged commit 104c3db into NixOS:master Jan 11, 2017
@abbradar
Copy link
Member

abbradar commented Jan 11, 2017

Late to the party but of possible interest to later changes:

  1. You can use "$(cat $NIX_CC/nix-support/dynamic-linker), for example: patchelf --interpreter "$(cat $NIX_CC/nix-support/dynamic-linker)";
  2. If you do not add mesa_noglu to the library path it should work as is. On NixOS this is handled with LD_LIBRARY_PATH. On conventional Linux libGL.so.1 is found in standard system library paths. So is for macOS, I believe;

EDIT: Actually I'm not sure at all how things on macOS are, but they don't have libGL.so and don't even use ELF. I don't think you can make your expression compatible with macOS without a completely different version.

@kierdavis kierdavis deleted the unigine-valley branch November 24, 2017 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants