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

python.pkgs.glances: add graph support #53847

Closed
wants to merge 1 commit into from

Conversation

timokau
Copy link
Member

@timokau timokau commented Jan 12, 2019

Motivation for this change

Makes it possible to generate system monitoring graphs. As mentioned in the inline commit, enabling this by default would lead to a significant closure size incerase:

with graph: /nix/store/37wp9rz60sk3cv6hvq282hhgnl89yf73-python3.7-glances-3.0.2       348 277 920
w/o  graph: /nix/store/5m50ba3qlfb2bra5jj098hy4y1pmdqk5-python3.7-glances-3.0.2       128 647 560
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.

@timokau timokau requested a review from FRidh as a code owner January 12, 2019 14:48
@timokau
Copy link
Member Author

timokau commented Jan 12, 2019

@GrahamcOfBorg build python2.pkgs.glances python3.pkgs.glances

@FRidh
Copy link
Member

FRidh commented Jan 12, 2019

Is pygal needed during build-time of this package?

@timokau
Copy link
Member Author

timokau commented Jan 12, 2019

No, just runtime.

@FRidh
Copy link
Member

FRidh commented Jan 12, 2019

I don't see the point of adding options when one can simply include the library to an environment. It saves a rebuild.

@timokau
Copy link
Member Author

timokau commented Jan 12, 2019

True, but

  • the user has to know which package is needed for which functionality
  • adding it to the environment is impure
  • this is somewhat of a special case, since glances provides a binary (not sure why it is in python-packages, probably there is also a library part) and may not be part of a python environment

@FRidh
Copy link
Member

FRidh commented Jan 13, 2019

Either they need to know a package name, or they need to know a flag for which they need to look at the expression, there isn't much difference.

I think a better solution here is to provide a glances derivation in all-packages.nix that composes an environment. Judging from the README there are a lot of optional dependencies. One could have something similar as say home-assistant. The Python package of glances itself could then stay either here or be moved.

Either python.withPackages is used to build the env, and then another derivation that symlinks only the parts of interest, or PYTHONPATH is set, using makePythonPath. I think here the latter is easiest.

@timokau
Copy link
Member Author

timokau commented Jan 13, 2019

Either they need to know a package name, or they need to know a flag for which they need to look at the expression, there isn't much difference.

I'm hoping that flags / package configuration may become more discoverable in the future. It also scales better with multiple optional dependencies for a feature.

I think a better solution here is to provide a glances derivation in all-packages.nix that composes an environment. Judging from the README there are a lot of optional dependencies. One could have something similar as say home-assistant. The Python package of glances itself could then stay either here or be moved.

I like that solution (although what would be really nice would be some way to express runtimeDependencies that doesn't trigger rebuilds). But wouldn't it be confusing to have two glances packages?

@FRidh
Copy link
Member

FRidh commented Jan 14, 2019

I'm hoping that flags / package configuration may become more discoverable in the future. It also
scales better with multiple optional dependencies for a feature.

although what would be really nice would be some way to express runtimeDependencies that doesn't trigger rebuilds.

That directly means having a "wrapper" derivation, as I suggested now with glances.

What we would have then are:

  • checkInputs for testing
  • nativeBuildInputs for building
  • installInputs in order to install (satisfy setup.py requirements)
  • runtimeInputs for optional dependencies.

In principle we could get rid of installInputs if we tell setup.py to ignore dependencies. That, however, would make it harder to package because you need to check then yourself that you're making available all required runtimeInputs.

But wouldn't it be confusing to have two glances packages?

You could move the current glances expression and hide it in say a let...in expression.

@timokau
Copy link
Member Author

timokau commented Jan 14, 2019

[...]
What we would have then are:

* `checkInputs` for testing

* `nativeBuildInputs` for building

* `installInputs` in order to install (satisfy `setup.py` requirements)

* `runtimeInputs` for optional dependencies.

[...]

Isn't buildInputs (despite the name) already supposed to be
runtimeInputs? Its not enforced, but it should contain the runtime
dependencies.

But wouldn't it be confusing to have two glances packages?

You could move the current glances expression and hide it in say a let...in expression.

That would break existing setups though. Although I can't find any
documentation of using glances as a library, so it doesn't really belong
to python-packages in the first place.

@FRidh
Copy link
Member

FRidh commented Jan 15, 2019

Yes, installInputs is effectively buildInputs for required runtime dependencies. Something like setuptools and setuptools_scm go in nativeBuildInputs.

@timokau
Copy link
Member Author

timokau commented Jan 15, 2019

This was a thing I needed once and though it might be nice to upstream. Not a high priority for me and I'm not willing to put in more work right now.

Thanks for your review!

@timokau timokau closed this Jan 15, 2019
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