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

Recipe for argon2-cffi #2398

Merged
merged 18 commits into from
Jan 5, 2021
Merged

Recipe for argon2-cffi #2398

merged 18 commits into from
Jan 5, 2021

Conversation

Arjun-Somvanshi
Copy link
Contributor

What have I done so far?

In my code I have been using the pycryptodome lib which already has a recipe in p4a, since it seemed to have the same dependencies as argon2-cffi, that being: cffi and setuptools, I decided to use it's code for argon2's recipe. After I did the fork I added the dependency in my spec file. It is visible that it got downloaded (but probably is not getting built properly) but always fails when I run the app. This is the error I got on a logcat after the build was done:

      `working: Traceback (most recent call last):                                             �[0m
       working:   File "setup.py", line 13, in <module>                                        �[0m
       working:     from setuptools import find_packages, setup                                �[0m
       working: ModuleNotFoundError: No module named 'setuptools'                              �[0m

Command failed: /usr/bin/python3 -m pythonforandroid.toolchain create --dist_name=reminiscor --bootstrap=sdl2 --requirements=python3,kivy,pycryptodome,argon2-cffi --arch armeabi-v7a --copy-libs --color=always --storage-dir="/home/kivy/Reminiscor/.buildozer/android/platform/build-armeabi-v7a" --ndk-api=21

@inclement
Copy link
Member

Try using a CompiledComponentsPythonRecipe - you'll need to do something like this to get the argon C components to build.

@Arjun-Somvanshi
Copy link
Contributor Author

I tried to look into what is a CompiledComponentsPythonRecipe but this is what it says in the documentation:

Using a CompiledComponentsPythonRecipe

This is similar to a CythonRecipe but is intended for modules like numpy which include compiled but non-cython components. It uses a similar mechanism to compile with the right environment.

This isn’t documented yet because it will probably be changed so that CythonRecipe inherits from it (to avoid code duplication).

@Arjun-Somvanshi
Copy link
Contributor Author

Is this link relevant to what you think I should look for : (https://askpythonquestions.com/2020/12/13/kivy-python-for-android-how-to-create-custom-recipes/)

@obfusk
Copy link
Contributor

obfusk commented Jan 3, 2021

You may (also) need to set call_hostpython_via_targetpython = False; e.g.

from pythonforandroid.recipe import CompiledComponentsPythonRecipe


class Argon2Recipe(CompiledComponentsPythonRecipe):
    version = '20.1.0'
    url = 'https://github.com/hynek/argon2-cffi/archive/master.zip'
    depends = ['setuptools', 'cffi']
    call_hostpython_via_targetpython = False


recipe = Argon2Recipe()

url = 'https://github.com/hynek/argon2-cffi/archive/master.zip'
depends = ['setuptools', 'cffi']
call_hostpython_via_targetpython = False

Copy link
Contributor

Choose a reason for hiding this comment

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

flake8 requires an extra blank line 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.

okay

Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't like the fact that the "blank line" contains whitespace.

@Arjun-Somvanshi
Copy link
Contributor Author

From what I understand it's still not able to compile the c?

@Arjun-Somvanshi
Copy link
Contributor Author

From what I understand it's still not able to compile the c?

It can't find the argon.h file do I need to include the c code somehow?

@obfusk
Copy link
Contributor

obfusk commented Jan 3, 2021

It can't find the argon.h file do I need to include the c code somehow?

Looks like it uses a git submodule for the extras/libargon2 directory, which is not included in the .zip.

@obfusk
Copy link
Contributor

obfusk commented Jan 3, 2021

Try using url = 'git+https://github.com/hynek/argon2-cffi@{version}'. I think that will include submodules.

@Arjun-Somvanshi
Copy link
Contributor Author

It can't find the argon.h file do I need to include the c code somehow?

Looks like it uses a git submodule for the extras/libargon2 directory, which is not included in the .zip.

shall I make my own zip with the other repo included inside this one from: https://github.com/p-h-c/phc-winner-argon2/tree/62358ba2123abd17fccf2a108a301d4b52c01a7c

@Arjun-Somvanshi
Copy link
Contributor Author

Try using url = 'git+https://github.com/hynek/argon2-cffi@{version}'.

okay, sorry I typed my thing before seeing your comment

@obfusk
Copy link
Contributor

obfusk commented Jan 3, 2021

Try using url = 'git+https://github.com/hynek/argon2-cffi@{version}'. I think that will include submodules.

Actually, I think that should just be url = 'git+https://github.com/hynek/argon2-cffi'.

Normally, you should include {version} in the url, but the @{version} syntax for git urls is for pip, whereas p4a seems to use the recipe's .version instead (and check out that tag/branch).

@@ -3,7 +3,7 @@

class Argon2Recipe(CompiledComponentsPythonRecipe):
version = '20.1.0'
url = 'https://github.com/hynek/argon2-cffi/archive/master.zip'
url = 'url = 'git+https://github.com/hynek/argon2-cffi@{version}'
Copy link
Contributor

Choose a reason for hiding this comment

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

that doesn't look quite right :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is my first pr ever, sorry if it's a bit shabby.

Copy link
Contributor

Choose a reason for hiding this comment

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

np. though you may want to do a rebase and squash all those intermediate commits when it's working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah okay I will do that


class Argon2Recipe(CompiledComponentsPythonRecipe):
version = '20.1.0'
url = 'git+https://github.com/hynek/argon2-cffi@{version}'
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 you need to remove the @{version} here. Sorry.

Copy link
Contributor Author

@Arjun-Somvanshi Arjun-Somvanshi Jan 3, 2021

Choose a reason for hiding this comment

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

np. I didn't see you edited that message.

@Arjun-Somvanshi
Copy link
Contributor Author

Arjun-Somvanshi commented Jan 3, 2021

Okay, I don't quite understand what the issue is now? It seemed like it almost passed it this time though

@obfusk
Copy link
Contributor

obfusk commented Jan 3, 2021

I think the problem is that p4a calls setup_ext but not setup_clib.
Try adding build_cmd = 'build' to the recipe. That should call both.

@Arjun-Somvanshi
Copy link
Contributor Author

You may (also) want to try building the recipe on your own computer instead.
With buildozer, you can use e.g.

p4a.branch = develop
p4a.local_recipes = ./my-p4a-recipes

I don't understand how the p4a.local_recipes works, this is what I have so far:

# (str) python-for-android fork to use, defaults to upstream (kivy)
p4a.fork = Arjun-Somvanshi

# (str) python-for-android branch to use, defaults to master
p4a.branch = develop

# (str) python-for-android git clone directory (if empty, it will be automatically cloned from github)
#p4a.source_dir =

# (str) The directory in which python-for-android should look for your own build recipes (if any)
#p4a.local_recipes =

If I am using my own fork, then why does it need the local_recipes thing?

@Arjun-Somvanshi
Copy link
Contributor Author

Arjun-Somvanshi commented Jan 4, 2021

Incase you are wondering if I have done a build with those settings above in me .spec file, here is the output
build_output.txt

@obfusk
Copy link
Contributor

obfusk commented Jan 4, 2021

If I am using my own fork, then why does it need the local_recipes thing?

In that case it doesn't :)
I'm using it so that I don't need to use a fork (and keep it up to date) just to have an extra recipe.

@Arjun-Somvanshi
Copy link
Contributor Author

Arjun-Somvanshi commented Jan 4, 2021

Incase you are wondering if I have done a build with those settings above in me .spec file, here is the output
build_output.txt

Oh okay, so if I use a local recipe do I still need to specify the package in the requirement field? Also what do you think of that build output I uploaded, is that some mistake on my part?

@obfusk
Copy link
Contributor

obfusk commented Jan 4, 2021

This recipe seems to work:

from pythonforandroid.recipe import CompiledComponentsPythonRecipe


class Argon2Recipe(CompiledComponentsPythonRecipe):
    version = '20.1.0'
    url = 'git+https://github.com/hynek/argon2-cffi'
    depends = ['setuptools', 'cffi']
    call_hostpython_via_targetpython = False
    build_cmd = 'build'

    def get_recipe_env(self, arch):
        env = super().get_recipe_env(arch)
        env['ARGON2_CFFI_USE_SSE2'] = '0'
        return env


recipe = Argon2Recipe()

@obfusk
Copy link
Contributor

obfusk commented Jan 4, 2021

You may run into #2400 when testing that locally.

AndreMiras
AndreMiras previously approved these changes Jan 5, 2021
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and the follow up changes with the help of @inclement and @obfusk
Looking good, will merge after the CI builds green

@Arjun-Somvanshi
Copy link
Contributor Author

i'll do a rebase and squash the commits

@Arjun-Somvanshi
Copy link
Contributor Author

I just added the change in the url

@Arjun-Somvanshi
Copy link
Contributor Author

yeah, so as I thought, the libargon2 is missing hence we used that url from before, so this time I am gonna make the changes, squash the commits, and force push.

@Arjun-Somvanshi
Copy link
Contributor Author

I am getting a lot of merge conflicts even though I followed the steps to a git rebase properly

@AndreMiras
Copy link
Member

Thanks for giving it a try 👍
Weird for the rebase. I can't think of what would have made conflicts, but that's fine we'll try to deal without.
FYI what I would have tried is:

git fetch --all --prune
# squashing
git reset --soft HEAD~17
git commit --amend
# rebasing
git rebase upstream/develop # given the remote is called upstream

I'll try to at least squash from the GitHub interface at least because it allows to

@Arjun-Somvanshi
Copy link
Contributor Author

Arjun-Somvanshi commented Jan 5, 2021

Thanks for giving it a try 👍
Weird for the rebase. I can't think of what would have made conflicts, but that's fine we'll try to deal without.
FYI what I would have tried is:

git fetch --all --prune
# squashing
git reset --soft HEAD~17
git commit --amend
# rebasing
git rebase upstream/develop # given the remote is called upstream

I'll try to at least squash from the GitHub interface at least because it allows to
I'm sorry, I was trying to post the comment and closed the pr by mistake, the issue I think is that I mixed up the commits, I did a few directly from github instead of the command line

@Arjun-Somvanshi
Copy link
Contributor Author

Arjun-Somvanshi commented Jan 5, 2021

okay tests passed, no more commits now. Just gotta squash the commits from before.

@Arjun-Somvanshi
Copy link
Contributor Author

@AndreMiras I tried the git commands you suggested the git log on my local repo gives the 17 commits even after running those commands

@AndreMiras
Copy link
Member

No problem, thanks again for trying 👍

Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

All good thanks!

@AndreMiras AndreMiras merged commit aadcfc4 into kivy:develop Jan 5, 2021
@obfusk
Copy link
Contributor

obfusk commented Jan 5, 2021

git fetch --all --prune
# squashing
git reset --soft HEAD~17
git commit --amend
# rebasing
git rebase upstream/develop # given the remote is called upstream

Using an interactive rebase to squash always works for me:

git fetch --all
git rebase -i upstream/develop

@obfusk
Copy link
Contributor

obfusk commented Jan 5, 2021

yeah, so as I thought, the libargon2 is missing hence we used that url from before, so this time I am gonna make the changes, squash the commits, and force push.

An alternative solution would be to add a separate recipe for libargon2 and depend on that.
This might be a good idea anyway, since that can then be updated separately (and shared by other recipes).

@Arjun-Somvanshi
Copy link
Contributor Author

@obfusk just give me a few days, I am in the middle of my semester exams, I will get back to this and clean it up as well as try and do this the right way. :) Thanks a lot for your help.

@obfusk
Copy link
Contributor

obfusk commented Jan 5, 2021

just give me a few days, I am in the middle of my semester exams

np. There's no hurry :) Good luck!

I will get back to this and clean it up

The current version is squashed and merged. So it's already fine :)

as well as try and do this the right way.

There's nothing "wrong" now. Using a separate libargon2 recipe would just be a nice improvement IMHO. And should be a separate PR.

vesellov pushed a commit to vesellov/python-for-android that referenced this pull request Feb 10, 2021
* testing argon2-cffi
* Using CompiledComponentsPythonRecipe
* argon2-cffi recipe
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.

4 participants