python312Packages.plotly: cleanup, enable checks on darwin #347763
+17
−49
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 tasks
sarahec
suggested changes
Oct 10, 2024
| @@ -66,7 +65,7 @@ buildPythonPackage rec { | |||
| ipython | |||
| ipywidgets | |||
| which | |||
| orca | |||
| # orca # unable to locate binary, is this the right package? | |||
There was a problem hiding this comment.
Place orca and psutil in optional-dependencies per https://plotly.com/python/getting-started/#orca
Then take them out of the check inputs
There was a problem hiding this comment.
Also add kaleido to the check inputs and disable kaleido tests on Darwin (as they try to launch it as a subprocess). See my recommended changes on the previous PR.
There was a problem hiding this comment.
FYI The reason orca isn't working as a check input here is one of its dependencies isn't building on Darwin (libaio, which is Linux-specific.)
There was a problem hiding this comment.
nice finding that orca is legacy!
optional-dependencies is supposed to mirror the optional deps defined in the package manifest, which I can't see plotly doing that neither in the code nor in the produced METADATA file.
kaleido is already in dependencies and thus need not also be added to check inputs
| ]; | ||
| disabledTestPaths = [ | ||
| # unable to locate orca binary | ||
| "plotly/tests/test_orca/" |
Comment on lines
+87
to
+82
| "test_dependencies_not_imported" | ||
| "test_lazy_imports" |
There was a problem hiding this comment.
These fail because the correct libraries aren't available at check time. Put only the pytestCheckHook in nativeCheckInputs and the rest (save for optional dependencies) into checkInputs. That will fix these tests.
From the stdenv documentation:
Dependencies needed only to run tests are similarly classified between native (executed during build) and non-native (executed at runtime):
nativeCheckInputsfor test tools needed on $PATH (such as ctest) and setup hooks (for example pytestCheckHook)
checkInputsfor libraries linked into test executables (for example the qcheck OCaml package)
These dependencies are only injected when doCheck is set to true.
There was a problem hiding this comment.
tried it, doesn't fix the lazy loading tests. python checkInputs are imported on the build architecture and will thus break cross
|
pbsds
force-pushed
the
feat-cleanup-plotly-1728575454
branch
from
3568a4a
to
2160479
Compare
pbsds
changed the title
pbsds
force-pushed
the
feat-cleanup-plotly-1728575454
branch
from
2160479
to
6da24bc
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this are fixes i made for #347567, but i did not push it due to the eval backlog
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.