-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
python3Packages.dash: init at 1.8.0 #77837
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should roughly be a commit per package addition
pname = "dash-core-components"; | ||
version = "1.7.0"; | ||
|
||
src = fetchurl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already a helper for this, see https://github.com/NixOS/nixpkgs/blob/211ed174657712a00fd3bcabd590d2e7b7a25e10/pkgs/development/python-modules/requests/default.nix for an example
src = fetchurl { | |
src = fetchPypi { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonringer I initially gave up on fetchPypi
because it wasn't working for me. I just realized it's because the package name should have underscores instead of dashes. It's now fixed for all packages in 4f0952022dcd5314aa6b56a98901155110c3294a.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should roughly be a commit per package addition
@jonringer I'm sorry, I wasn't aware of that. Can I rebase and force push with a proper commit log? Ok so I force pushed a cleaned up commit log to fit that requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests (checkout from the github repos is preferable to no tets)
Please add yourself as a maintainer
Please apply comments to all packages
closes #69110
doCheck = false; | ||
|
||
meta = { | ||
description = "Dash is a Python framework for building analytical web applications. No JavaScript required."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need to define the subject, we know it's the description of a given package. "No JavaScript required." is just click-baity
description = "Dash is a Python framework for building analytical web applications. No JavaScript required."; | |
description = "Python framework for building analytical web applications"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 411a351
meta = { | ||
description = "Dash is a Python framework for building analytical web applications. No JavaScript required."; | ||
homepage = https://dash.plot.ly/; | ||
license = with lib.licenses; [ mit ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs other suggestion to work
license = with lib.licenses; [ mit ]; | |
license = licenses.mit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 411a351
# No tests in archive | ||
doCheck = false; | ||
|
||
meta = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also useful for referencing maintainers
meta = { | |
meta = with lib; { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Done in 411a351
I am sorry to be such a newbie but can you expand on this @jonringer? Would the following be enough? # in lieu of proper tests
pythonImportsCheck = [
"dash"
]; Thank you for your patience |
EDIT: However, unit tests are a much better measure to ensure the packages are internally correct and usable. |
So I take it it would be good enough? |
I was a newbie at nix not too long ago as well. No reason to apologize :) |
The big push for tests is to ensure that a bunch of packages aren't subtly broken. Since the sources are available through github, it's preferred to use from source rather just have our fingers crossed. This becomes more important when we have to alter upstream version bounds, having tests ensures that we have valid packages, even if upstream isn't testing against those dependency ranges. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the add antoinerg as maintainer of python3Packages.dash-*
commit, do you mind just doing:
antoinerg = {
email = "roygobeil.antoine@gmail.com";
github = "antoinerg";
githubId = 301546;
name = "Antoine Roy-Gobeil";
};
as maintainers: add antoinerg
then adding yourself to the other packages can remain as another commit.
Generally maintainer additions are separate from package changes.
Not sure how strong your git-fu is, let me know if you need help with altering git history. (but i would look at how to use git rebase -i
if you're not already familiar)
With 9cd2e23, it now runs a few unit tests. I skipped the ones that require browsers. All those tests are run upstream prior to a release. I hope this is good enough @jonringer ! |
please squash the dash fixup commit into the other.
git push --force otherwise LGTM |
Now done @jonringer ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff LGTM
[21 built, 36 copied (194.3 MiB), 12.9 MiB DL]
https://github.com/NixOS/nixpkgs/pull/77837
15 package built:
python27Packages.dash python27Packages.dash-core-components python27Packages.dash-html-components python27Packages.dash-renderer python27Packages.dash-table python37Packages.dash python37Packages.dash-core-components python37Packages.dash-html-components python37Packages.dash-renderer python37Packages.dash-table python38Packages.dash python38Packages.dash-core-components python38Packages.dash-html-components python38Packages.dash-renderer python38Packages.dash-table
@GrahamcOfBorg build python27Packages.dash python27Packages.dash-core-components python27Packages.dash-html-components python27Packages.dash-renderer python27Packages.dash-table python37Packages.dash python37Packages.dash-core-components python37Packages.dash-html-components python37Packages.dash-renderer python37Packages.dash-table python38Packages.dash python38Packages.dash-core-components python38Packages.dash-html-components python38Packages.dash-renderer python38Packages.dash-table |
Motivation for this change
Dash is an open-source framework that creates analytical web apps from Python without writing a single line of javascript (https://dash.plot.ly/). It would be awesome to package it in NixOS!
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)Manual test
I tested that I could start a simple Dash app:
$ nix-shell -I nixpkgs=$(pwd) -p python3Packages.dash $ python app.py