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

pythonPackages.moderngl: init at 5.5.0 #54737

Merged
merged 1 commit into from Feb 11, 2019
Merged

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.

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.

pkgs/development/python-modules/moderngl/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/moderngl/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/moderngl/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/moderngl/default.nix Outdated Show resolved Hide resolved
@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.


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.

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants