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
electrum: 3.2.4 -> 3.3.2 plus new dependencies #54466
Conversation
LGTM. Thank you!
Ran result/bin/electrum and created a test wallet. Works for me. |
@GrahamcOfBorg build electrum |
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 the aio libraries are missing tests, maybe they are not included in the Pypi package
Yes, the tests/ directory is missing in both tarballs. Please add doCheck = false
and a comment with the reason.
Also don't forget to squash your commits such that only three are remaining. |
@dotlambda I could also fetch the libraries from GitHub to get the tests |
That's up to you. However, I suspect at least the aiohttp-socks tests to fail due to accessing the network. |
@dotlambda even worse the tests depend on a binary contained in the repo |
@dotlambda I enabled the tests for electrum but I am not sure if it will work on MacOS. Could you trigger a borg build ? |
I will squash commits once everything is validated |
@GrahamcOfBorg build electrum |
|
||
propagatedBuildInputs = [ aiohttp ]; | ||
|
||
# Checks needs internet access |
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 don't think so, but they use localhost, to which access is denied in the sandbox.
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 a remote address ? https://github.com/romis2012/aiohttp-socks/blob/3252f4bdd37fb9a7360481977f800189cb3e3aca/tests/test_socks.py#L18
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.
Indeed, it is. Sorry.
|
||
disabled = pythonOlder "3.6"; | ||
|
||
# Checks needs internet access |
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.
no
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 spotted a remote address used here : https://github.com/kyuupichan/aiorpcX/blob/d8568f6c1865d1927034e05807a412b08c6d7526/tests/test_socks.py#L15
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.
LGTM, apart from squashing the commits. Did not test any functionality.
Co-authored-by: nyanloutre <paul@nyanlout.re>
Co-authored-by: nyanloutre <paul@nyanlout.re>
Co-authored-by: nyanloutre <paul@nyanlout.re>
11ea2d7
to
2b01bc5
Compare
@dotlambda do you want it to be more squashed ? |
The last two could be squashed together :) |
2b01bc5
to
3c38d22
Compare
done |
Thanks a lot! |
Motivation for this change
Previous PR was closed, I applied required fixes
⚠️ I did not test execution at this time
Also the aio libraries are missing tests, maybe they are not included in the Pypi package
cc @clefru @dotlambda
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)