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

Jump to bottom

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

Merged
merged 1 commit into from Dec 20, 2019
Merged

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

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

@nh2
haskellPackages.GLUT: Fix freeglut.pc -> glut.pc.
4bf23ce 
This was broken by PR NixOS#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:

    NixOS#70235 (comment)
@nh2
Copy link
Contributor Author

nh2 commented Dec 20, 2019

@GrahamcOfBorg build

This was referenced Dec 20, 2019
Merged
Closed
@barryfm
Copy link
Contributor

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

barryfm commented Dec 20, 2019

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

cdepillabout
cdepillabout approved these changes Dec 20, 2019
View reviewed changes
Copy link
Member

@cdepillabout cdepillabout left a comment •
edited

Choose a reason for hiding this comment

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

@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 commented Dec 20, 2019

This issue should be fixed upstream.

@nh2
Copy link
Contributor Author

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

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

@barryfm
Copy link
Contributor

barryfm commented Dec 20, 2019 •
edited

@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 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 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
@nh2
Copy link
Contributor Author

nh2 commented Dec 24, 2019

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

nixpkgs/pkgs/development/haskell-modules/configuration-nix.nix

Lines 467 to 477 in 3087143

# 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

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

@barryfm barryfm mentioned this pull request Jan 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdepillabout cdepillabout cdepillabout approved these changes

@basvandijk basvandijk Awaiting requested review from basvandijk

Assignees
No one assigned
Labels
6.topic: haskell 10.rebuild-darwin: 0 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nh2 @barryfm @peti @turboMaCk @cdepillabout