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

Fix dependency issues (and fastecdsa windows depencency) #380

Merged
merged 2 commits into from
Dec 23, 2019

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Dec 16, 2019

+add: p2pclient, pexpect, eth_utils to setup.py
+add: custom fastecdsa strategy in regards to windows
+bump: fastecdsa from 1.7.4 to 1.7.5
(only changes are 2 lines over here, in point.py)

What was wrong?

Default pip install -e .[dev] did not result in complete installation of all dependencies needed for normal development.

#363, windows installations and development fails because fastecdsa does not have a wheel, nor a easily-installable library by default.

How was it fixed?

Adding 3 extra dependencies in test and dev, correspondent with:

adding p2pclient, pexpect from tox.ini

adding eth_utils from github issue template

Furthermore, adding a modded version of fastecdsa from https://github.com/shadowjonathan/fastecdsa-any when a windows platform is detected.

fastecdsa does currently not support nor build a windows wheel, but I have found a (probable) way to make installation succeed with the right libraries.

Warning: this implementation is not tested, I am adding that modded version to this library exactly to test and find problems with a build, and if fastecdsa is viable for libp2p in the end.
(see AntonKueltz/fastecdsa#11)

Lastly, fastecdsa is bumped to 1.7.5 for uniformity with the modded windows version, a diff between the two versions revealed to me that these two lines are the only things that changed between the versions. Please test and verify if this does not break things.

To-Do

  • Clean up commit history
  • Extensive testing on windows platforms to justify inclusion of modded version
  • Testing of fastecdsa==1.7.5 on all platforms to assure nothing is broken

Cute Animal Picture

put a cute animal picture link inside the parentheses

@ShadowJonathan ShadowJonathan marked this pull request as ready for review December 16, 2019 11:02
@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Dec 16, 2019

...I realise I am basing this pull off of an outdated master branch, as I cannot put this PR into draft status again, I am asking everyone that reads this to hold on a moment.

Edit: Nevermind, fixed.

setup.py Outdated Show resolved Hide resolved
setup.py Outdated
@@ -33,6 +35,7 @@
"ipython",
"setuptools>=36.2.0",
"tox>=3.13.2,<4.0.0",
"eth_utils",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need eth_utils here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the issue template, it's asked to run python -m eth_utils, it'll be useful to have that as a part of a default development suite.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's the typo in the issue template and we should fix it. IMO we shouldn't have eth_utils here.

setup.py Outdated
# See the following issues for more information;
# https://github.com/libp2p/py-libp2p/issues/363
# https://github.com/AntonKueltz/fastecdsa/issues/11
"fastecdsa@git+https://github.com/shadowjonathan/fastecdsa-any@"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind having a PR in AntonKueltz/fastecdsa to fix the issue there, and then we depend on their fixed release? IIRC, PyPI does not accept this kind of git dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already notified AntonKueltz/fastecdsa about this, but it is going slow, I'm adding this dependency specifically for development on windows.

The alternative is to make a pypi package called fastedcsa-any, which'll contain two wheels (win32, win_amd64) so windows installation actually works without it resorting to trying to build a wheel from scratch.

Should I impliment the latter one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've uploaded python wheels for fastecdsa to pypi: fastecdsa-any
These are built for for windows (32-bit and 64-bit) python 3.6 through 3.8, library version 1.7.4 and 1.7.5 in a matrix, so I suggest replacing this git+http notion (pypi accepts any kind of dependency, it does not care for dependencies, only uploaded tarballs and files) with fastecdsa-any for windows systems specifically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current requirement works for me(fastecdsa-any==1.7.5 for windows and fastecdsa==1.7.5 for the rest of the platforms). We can switch to something like fastecdsa>=1.7.6 after they support windows.

add fastecdsa-any requirements
@ShadowJonathan
Copy link
Contributor Author

Force-pushed changes to the branch to simplify the git history, the original branch (with original commits) can be found on fix_dependencies_old

setup.py Outdated
@@ -33,6 +35,7 @@
"ipython",
"setuptools>=36.2.0",
"tox>=3.13.2,<4.0.0",
"eth_utils",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's the typo in the issue template and we should fix it. IMO we shouldn't have eth_utils here.

Copy link
Contributor

@mhchia mhchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mhchia mhchia merged commit 9d3312e into libp2p:master Dec 23, 2019
@ShadowJonathan ShadowJonathan deleted the fix_dependencies branch December 23, 2019 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants