Groestlcoin hash #1288
Groestlcoin hash #1288
Conversation
tshirtman
force-pushed
the
groestlcoin_hash
branch
2 times, most recently
from
c4e4c0c to
c21de6e
Compare
| ndk_path = os.path.join( | ||
| ndk_dir, 'sources', 'python', python_version, 'libs', arch.arch | ||
| ) | ||
| env['LDFLAGS'] = ' -L {}'.format(ndk_path) + env['LDFLAGS'] |
There was a problem hiding this comment.
This change makes sense, but I don't understand why it wouldn't be needed by other CythonRecipes.
There was a problem hiding this comment.
Agree, in fact I've been adding it in all my recipes e.g. https://github.com/AndreMiras/EtherollApp/blob/v20180517/src/python-for-android/recipes/scrypt/__init__.py#L26
That and also maybe the -I header path like here https://github.com/kivy/python-for-android/pull/1250/files#diff-569e13021e33ced8b54385f55b49cbe6
There was a problem hiding this comment.
The include dir is already included by CythonRecipe, at https://github.com/kivy/python-for-android/blob/master/pythonforandroid/recipe.py#L1045
There was a problem hiding this comment.
Looks good to me - I'm glad that the CythonRecipe worked almost out of the box, with only this one env var that can probably be added to the CythonRecipe itself.
I'll make some comments on the openssl PR, I guess once that's merged it will be easier to come back and stick this one in.
|
If the pr #1217 is not going to be merged, i might rebase the PR on master instead to simplify the job of the users of these PR, as they would only need one recipe to patch, not two? Since this doesn't technically depends on it, and could be used separately. |
tshirtman
force-pushed
the
groestlcoin_hash
branch
from
c21de6e to
8ea59a9
Compare
|
So i did the rebase, and additionally, i moved the get_recipe_env patch to CythonRecipe, as the comments seemed to agree on the usefulness of that. Didn't test, but that seems straightforward, if the tests pass, we should be fine, right? :D |
|
Yes the recipe looks straightforward indeed, make sure tests are passing. Basically just fix the PEP8 and let's see 🤞 |
|
Maybe i missed something, i looked if there was recent changes to CythonRecipe and didn't see anything more recent, so i applied it directly, maybe there was something elsewhere? |
|
Yes I was thinking maybe it's already covered by the recent changes to |
|
Thanks for the update, we're getting closer 🥂 |
AndreMiras
force-pushed
the
groestlcoin_hash
branch
2 times, most recently
from
76585ca to
bd399dd
Compare
This recipe depends on python3crystax, and has only been tested with python3.6 crystax, but should work with 3.5 too. Tested under provided Dockerfile and compiled fine with: ``` python setup_testapp_python3.py apk --sdk-dir /opt/android/android-sdk --ndk-dir /opt/android/crystax-ndk --requirements python3crystax,setuptools,android,groestlcoin_hash ```
AndreMiras
force-pushed
the
groestlcoin_hash
branch
from
bd399dd to
9316d3d
Compare
|
Comments addressed and compiled fine in my Docker container. |
pythonforandroid/recipe.py
Outdated
| @@ -1068,6 +1068,12 @@ def get_recipe_env(self, arch, with_flags_in_cc=True): | |||
| env['LIBLINK_PATH'] = liblink_path | |||
| ensure_dir(liblink_path) | |||
|
|
|||
| ndk_dir = self.ctx.ndk_dir | |||
| python_version = '.'.join(self.ctx.python_recipe.version.split('.')[:2]) | |||
| ndk_path = os.path.join( | |||
There was a problem hiding this comment.
Looking great, please simply fix the other import error introduced 😕
https://travis-ci.org/kivy/python-for-android/jobs/423234368
pythonforandroid/recipe.py:1073:20: F821 undefined name 'os'
FYI you can run tox in your local copy to check theses before Travis does so for you.
Simply replace os.path.join by simply join since that's the one being already imported here https://github.com/kivy/python-for-android/blob/cbe9e8e/pythonforandroid/recipe.py#L1
Also it would be awesome if you could squash all three commits into one. I can deal with both things if you give me write access to your PR.
| from pythonforandroid.recipe import CythonRecipe | ||
|
|
||
|
|
||
| class groestlcoin_hashRecipe(CythonRecipe): |
There was a problem hiding this comment.
Looking good! Maybe we should make it a bit Pythonic by calling it GroestlcoinHashRecipe what do you think?
| class groestlcoin_hashRecipe(CythonRecipe): | ||
| version = '1.0.1' | ||
| url = 'https://pypi.python.org/packages/source/g/groestlcoin_hash/groestlcoin_hash-{version}.tar.gz' # noqa | ||
| depends = ['python3crystax'] |
There was a problem hiding this comment.
I have to say, I like NOT having Python2 support just like you did, well done! I think we should all go to that direction and slowly drop it 👍
pythonforandroid/recipe.py
Outdated
| ndk_path = os.path.join( | ||
| ndk_dir, 'sources', 'python', python_version, 'libs', arch.arch | ||
| ) | ||
| env['LDFLAGS'] = ' -L {}'.format(ndk_path) + env['LDFLAGS'] |
There was a problem hiding this comment.
Minor remark, for records, as suggested in the other conversation, maybe this now already covered by https://github.com/kivy/python-for-android/pull/793/files#diff-ba2a2345fbe0c6c42bd6eb8536bf4564
But not a big deal living it here 😉
Adds a recipe for groestlcoin_hash, a hash algorithm used by the Groestlcoin cryptocurrency.
This PR depends on #1217 because as with any electrum fork, the app using this recipe uses this patched p4a to allow using ssl with python3crystax, so this recipe wasn't tested on p4a master.
The overriden
get_recipe_envcould maybe be removed and the change it does be backported to theCythonRecipe.get_recipe_envmethod, at the same place `-lpython{}m' is added, so other recipes could benefit from it. I don't know if it could potentially lead to issues with others though, so that's an open question for me.