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.

Sign up for GitHub

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

pythonPackages.moderngl: init at 5.5.0 #54737

Merged
merged 1 commit into from
Feb 11, 2019
Merged

pythonPackages.moderngl: init at 5.5.0 #54737

merged 1 commit into from
Feb 11, 2019

Conversation

c0deaddict
Copy link
Member

Motivation for this change

Library is missing from Nixpkgs.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@c0deaddict c0deaddict requested a review from FRidh as a code owner January 27, 2019 23:12
@GrahamcOfBorg GrahamcOfBorg added 6.topic: python 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jan 27, 2019
dotlambda
dotlambda requested changes Jan 28, 2019
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Ran 0 tests in 0.000s

Plase add an appropriate checkPhase or - if there are no tests - set doCheck = false and add a comment with the reason.

@c0deaddict
Copy link
Member Author

Thanks for reviewing!

Good catch about the missing tests, hadn't noticed that. The PyPi tarball/zip apparently doesn't contain any tests. Digging a bit deeper I found this closed merge request: #40767 I tried that route (fetching sources from github and explicitly running pytest) but that gave me the same error as @PsyanticY mgl.Error: cannot detect the display

I'm not 100% sure, but I think the library can't run without a X display. Seems logical since the library probably tries to run some OpenGL commands on the GPU. My conclusion is that this library is not testable in the nix build environment. Does that reasoning make sense? Is it okay to skip tests for this reason?

I have tested the library by running the examples from the ModernGL github.

@c0deaddict
Copy link
Member Author

Ping @dotlambda I've made the changes you requested.

@dotlambda
Copy link
Member

@GrahamcOfBorg build python3.pkgs.moderngl

@dotlambda
Copy link
Member

Is it okay to skip tests for this reason?

It is.

dotlambda
dotlambda reviewed Feb 7, 2019
View reviewed changes
pkgs/development/python-modules/moderngl/default.nix Outdated

disabled = !isPy3k;

buildInputs = [ libGLU_combined libX11 ];
Copy link
Member

Choose a reason for hiding this comment

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

In https://aur.archlinux.org/packages/python-moderngl-git/, they only use libGL. What did you infer these dependencies from?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried a build without libX11 and it crashed with:

/nix/store/34kliyybc28hydrxv4f7l0h9p6wm6dhp-libGLU-combined/include/GL/glx.h:30:10: fatal error: X11/Xlib.h: No such file or directory
 #include <X11/Xlib.h>
          ^~~~~~~~~~~~
compilation terminated.
error: command 'gcc' failed with exit status 1

Copy link
Member

Choose a reason for hiding this comment

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

Alright, I get the same. However, you should replace libGLU_combined by libGL or is there a reason not to?

Copy link
Member

Choose a reason for hiding this comment

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

And we should mark it as broken on Darwin as long as that doesn't work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested it with libGL and that works just fine. I've removed Darwin from the platforms, added a comment that it is broken on that platform.

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Feb 11, 2019
@dotlambda
Copy link
Member

@GrahamcOfBorg build python3.pkgs.moderngl

@dotlambda dotlambda merged commit 3133d59 into NixOS:master Feb 11, 2019
@c0deaddict
Copy link
Member Author

Thanks for reviewing @dotlambda!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotlambda dotlambda dotlambda requested changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@c0deaddict @dotlambda @GrahamcOfBorg