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

haskellPackages.GLUT: Fix freeglut.pc -> glut.pc #76073

Merged
merged 1 commit into from Dec 20, 2019

Conversation

@nh2
Copy link
Contributor

@nh2 nh2 commented Dec 20, 2019

Motivation for this change

See #70235 (comment)

This was broken by PR #70235 with commit:

  • f5ae5ca - freeglut: 3.0.0 -> 3.2.1

The in the newer freeglut version, the pkg-config file is called glut.pc, no longer freeglut.pc.

Found in:

Fixes error:

Setup: The pkg-config package 'freeglut' is required but it could not be found.
Things done
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @peti

FYI @bjornfor @barryfm @jonringer

This was broken by PR #70235 with commit

    f5ae5ca - freeglut: 3.0.0 -> 3.2.1

The in the newer freeglut version, the pkg-config file is called
`glut.pc`, no longer `freeglut.pc`.

Found in:

    #70235 (comment)
@nh2 nh2 requested review from basvandijk and cdepillabout as code owners Dec 20, 2019
@nh2
Copy link
Contributor Author

@nh2 nh2 commented Dec 20, 2019

@barryfm
Copy link

@barryfm barryfm commented Dec 20, 2019

Isn't this really a issue with freeglut? If the package is called freeglut but provides libglut and a glut.pc pkgconf file, isn't this inconsistency the issue?

Freeglut has the option of building with the FREEGLUT_REPLACE_GLUT=OFF and produce a freeglut.pc and libfreeglut libraries. Or it can just add to freeglut/default.nix

postFixup = ''
ln -s glut.pc $dev/lib/pkgconfig/freeglut.pc
'';

Or (I think) the package could be named glut with a freeglut alias added to nixpkgs. Otherwise I suspect more than just Haskell's GLUT package will have to change, and we have another gratuitous inconsistency between version.

My novice instincts says we should follow Arch Linux's choice to just add the freeglut.pc link.

@nh2
Copy link
Contributor Author

@nh2 nh2 commented Dec 20, 2019

My novice instincts says we should follow Arch Linux's choice to just add the freeglut.pc link.

@barryfm I have nothing against that, but i think my PR is still correct, because the Haskell GLUT package seems to support any GLUT implementation, freeglut or normal glut, so I think it makes sense that the nixpkgs patch makes it search for a general glut.pc.

@barryfm
Copy link

@barryfm barryfm commented Dec 20, 2019

@nh2, you're right. Sorry for my outburst.

Copy link
Member

@cdepillabout cdepillabout left a comment

@nh2 Thanks for fixing this!

This fix seems reasonable.

Feel free to merge this in if you're satisfied with it.

@peti
Copy link
Member

@peti peti commented Dec 20, 2019

This issue should be fixed upstream.

@nh2
Copy link
Contributor Author

@nh2 nh2 commented Dec 20, 2019

This issue should be fixed upstream.

@peti Which upstream? Did you seem my comment on #70235 (comment)? We in nixpkgs add the wrong name with our patch.

@peti
Copy link
Member

@peti peti commented Dec 20, 2019

Which upstream?

The authors of the Haskell package, e.g. https://github.com/haskell-opengl/GLUT/issues.

We in nixpkgs add the wrong name with our patch.

What concerns me is that we have to patch that Cabal file in the first place. Why is that even necessary? It looks to me like upstream should add that dependency to their Cabal file.

@turboMaCk
Copy link
Member

@turboMaCk turboMaCk commented Dec 20, 2019

@peti Please check discussion under this commit. 2228100#commitcomment-18701401 I think this indeed might be an NixOS specific issue.

@barryfm
Copy link

@barryfm barryfm commented Dec 20, 2019

@peti, I have compared the 19.09 with unstable, and freeglut going from 3.0.0 to 3.2.1 caused the
pkg-config file to be intentionally renamed from freeglut.pc to glut.pc during the install. This is not a backward compatible change.

This is controlled by freeglut's cmake option FREEGLUT_REPLACE_GLUT defaulting to ON (unless on WIndows). Previously this flag did not trigger this rename.

I'm not sure if NixOS has a policy about such situations, but it would seem to make sense that a uniform way was chosen to deal with package names not being the same than the dependency they provide.

@peti
Copy link
Member

@peti peti commented Dec 20, 2019

I don't understand how the upstream package can do anything useful without depending on glut (or freeglut). My guess is that it won't even compile (which is why we added that dependency in our repository). So why is that dependency not included in the upstream Cabal file? Why do we have to add it in the first place?

Am I missing something?

@peti
Copy link
Member

@peti peti commented Dec 20, 2019

Ok. @turboMaCk has pointed out that the Haskell library contains hacks that load libglut.so at run-time (or rather, try to load it) and therefore they don't need a dependency at build-time on the library since they load it themselves. We, however, need that build-time dependency so that our linker scripts can figure out the appropriate RPATH set to allow the library to find libglut.so when it tries to load it.

The relevant code is at https://github.com/haskell-opengl/GLUT/blob/master/cbits/HsGLUT.c.

In the light of this, I don't see a reasonable chance to convince upstream to declare a proper dependency. I guess this means that we can just merge this update and forget about it ... until next time.

@peti peti merged commit 7ac673d into NixOS:master Dec 20, 2019
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
haskellPackages.GLUT on aarch64-linux Success
Details
haskellPackages.GLUT on x86_64-linux Success
Details
@nh2
Copy link
Contributor Author

@nh2 nh2 commented Dec 24, 2019

I thought that the above should be documented, but found that it is already documented here:

# GLUT uses `dlopen` to link to freeglut, so we need to set the RUNPATH correctly for
# it to find `libglut.so` from the nix store. We do this by patching GLUT.cabal to pkg-config
# depend on freeglut, which provides GHC to necessary information to generate a correct RPATH.
#
# Note: Simply patching the dynamic library (.so) of the GLUT build will *not* work, since the
# RPATH also needs to be propagated when using static linking. GHC automatically handles this for
# us when we patch the cabal file (Link options will be recored in the ghc package registry).
#
# Additional note: nixpkgs' freeglut and macOS's OpenGL implementation do not cooperate,
# so disable this on Darwin only
${if pkgs.stdenv.isDarwin then null else "GLUT"} = addPkgconfigDepend (appendPatch super.GLUT ./patches/GLUT.patch) pkgs.freeglut;

@turboMaCk
Copy link
Member

@turboMaCk turboMaCk commented Dec 24, 2019

yes this is what @peti found out on stream as well. Unfortunetely things like this are not visible in the diffs which complicates discussions under PRs a bit..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.