-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
slither-analyzer: 0.10.3 -> 0.10.4 | python312Packages.web3: 6.5.0 -> 6.20.3 | python312Packages.pyunormalize: init at 15.1.0 | python312Packages.prettytable: 3.10.1 -> 3.10.2 #337542
base: master
Are you sure you want to change the base?
Conversation
b89a2bd
to
486621a
Compare
@ofborg build slither-analyzer again. |
486621a
to
e9c948b
Compare
$ nixpkgs-review pr 337542 --print-result --build-args "--max-jobs 0 --builders @${HOME}/.config/nix/machines" Result of 4 packages marked as broken and skipped:
20 packages failed to build:
199 packages built:
It doesn't seem that those broken packages are caused by this changset. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4469 |
}; | ||
|
||
# do not run tests | ||
doCheck = false; |
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.
Upstream repo seem to contain tests.
Please add pythonImportsCheck
.
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. Please check everything again.
be17bc6
to
b7ffd50
Compare
@ofborg build slither-analyzer |
b7ffd50
to
706a8c1
Compare
hash = "sha256-z0qHRRoPHLdpEaqX9DL0V54fVkovDITOSIxzpzkBtsE="; | ||
}; | ||
|
||
doCheck = true; |
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.
Isn't this the default?
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.
Yes, it seems so. I wasn't sure about it myself, and I thought it would be better for the maintainers to not rely on implicit knowledge. I could also remove it, it doesn't seem to be a very unified thing in the code base:
$ git grep 'doCheck = true' pkgs/development/python-modules| wc -l
94
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 remove it.
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.
okay, removed.
706a8c1
to
8044b70
Compare
}: | ||
|
||
buildPythonPackage rec { | ||
pname = "web3"; | ||
version = "6.5.0"; | ||
version = "6.20.3"; | ||
format = "setuptools"; |
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.
Can we also convert this to pyproject?
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.
By doing so, these errors popped up:
> - ckzg not installed
> - hexbytes<0.4.0,>=0.1.0 not satisfied by version 1.2.0
> - lru-dict<1.3.0,>=1.1.6 not satisfied by version 1.3.0
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.
And as we speak web3 7 doesn't require ckzg anymore.... :/
Upstreams runs faster than the speed of PR.
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.
Shall we call it for a day, and fix web3 pyproject project in a future PR? I am happy to do it, and I will keep this in mind.
679f167
to
c652f99
Compare
setuptools-scm | ||
]; | ||
|
||
propagatedBuildInputs = [ | ||
propagatedBuildInputs = with python3Packages; [ |
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.
propagatedBuildInputs = with python3Packages; [ | |
dependencies = with python3Packages; [ |
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
]; | ||
|
||
pythonImportsCheck = [ "pyunormalize" ]; | ||
|
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 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.
I thought the tests were enabled by default. I was asked to remove "doCheck = true;" in the previous comments
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 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.
Did you read the documentation I showed you?
It says that buildPythonPackage
sets doCheck = true
by default, and the package needs its own checkPhase. In most cases, just add pytestCheckHook to nativeCheckInputs.
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.
Got it. Done.
@@ -29,32 +30,29 @@ buildPythonPackage rec { | |||
owner = "ethereum"; | |||
repo = "web3.py"; | |||
rev = "v${version}"; | |||
hash = "sha256-RNWCZQjcse415SSNkHhMWckDcBJGFZnjisckF7gbYY8="; | |||
hash = "sha256-lxCd1Cc79SW8uQrKom9Lqeb7VPuj2nKFlCj51EgSSuk="; | |||
}; | |||
|
|||
# Note: to reflect the extra_requires in main/setup.py. | |||
passthru.optional-dependencies = { |
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.
passthru.optional-dependencies = { | |
optional-dependencies = { |
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
requests | ||
websockets | ||
]; | ||
propagatedBuildInputs = [ |
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.
propagatedBuildInputs = [ | |
dependencies = [ |
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
afc033f
to
1e3bcee
Compare
what should we do next? |
1 similar comment
what should we do next? |
1e3bcee
to
f571712
Compare
converting this to draft, due to that another PR that addresses web3 package issue is currently ongoing #342749 |
Description of changes
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.