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

recipes: Introduce RustCompiledComponentsRecipe, add pydantic-core and update cryptography #2962

Merged
merged 1 commit into from Mar 31, 2024

Conversation

T-Dynamos
Copy link
Contributor

@T-Dynamos T-Dynamos commented Jan 25, 2024

Closes #2661 #2955
Surpasses #2957

CC: @AndreMiras @misl6 Is approach used here is fine?

@T-Dynamos
Copy link
Contributor Author

@Julian-O Hi, can you please triggre CI?

@Julian-O
Copy link
Contributor

Sorry, I don't have permissions for that. @misl6?

@AndreMiras
Copy link
Member

Interesting approach, thanks for looking this up.
The CI actually tried running, but failed because of a syntax error:

[Invalid workflow file: .github/workflows/push.yml#L232](https://github.com/kivy/python-for-android/actions/runs/7658579349/workflow)
The workflow is not valid. .github/workflows/push.yml (Line: 232, Col: 13): Unexpected symbol: '|'. Located at position 23 within expression: (
  github.event_path | contains('pydantic-core')
  # github.event_path | contains('dir2') ||
  # github.event_path | contains('dir3')
)

See here https://github.com/kivy/python-for-android/actions/runs/7658579349
This would be our first Rust recipe I think. It's probably OK that we don't generalize anything yet, but we may want as we get more of them. I don't have much knowledge about Rust, so I don't know if the current method is future proof for dealing with different compiler versions plus dependencies, but I'm fine with it as it's a first, as long as the CI is happy about it.
misl6 should have the last word as I haven't been very active in p4a for a long time

@T-Dynamos T-Dynamos force-pushed the rust-backend branch 3 times, most recently from a4773bc to 024fad9 Compare January 26, 2024 12:38
@T-Dynamos T-Dynamos marked this pull request as draft January 26, 2024 12:38
@T-Dynamos T-Dynamos marked this pull request as ready for review January 26, 2024 12:57
@T-Dynamos T-Dynamos force-pushed the rust-backend branch 9 times, most recently from 54ac6f4 to b2182e1 Compare January 26, 2024 14:43
@T-Dynamos
Copy link
Contributor Author

Build works, please somebody cancel all the previous actions triggred except the latest one.

@misl6
Copy link
Member

misl6 commented Jan 28, 2024

@T-Dynamos FYI, Unit tests are failing.

Regarding the approach, there's something that will be repetitive along all the upcoming recipes for rust-backed libraries?
If yes, how about moving these things into a RustCompiledComponentsRecipe ?

@T-Dynamos
Copy link
Contributor Author

T-Dynamos commented Jan 28, 2024

RustCompiledComponentsRecipe

It's good idea. As cryptography is also built on rust.

@misl6
Copy link
Member

misl6 commented Jan 28, 2024

RustCompiledComponentsRecipe

It's good idea. As cryptography is also built on rust.

Do not take it as a "we should do it like that", feel free to improve the thinking.

Please also remind to add the new dependency into prerequisites.py, and in docs.

@T-Dynamos T-Dynamos marked this pull request as draft January 28, 2024 18:55
@T-Dynamos T-Dynamos changed the title recipes: new pydantic-core recipe recipes: Introduce RustCompiledComponentsRecipe, add pydantic-core and update cryptography Jan 29, 2024
@T-Dynamos T-Dynamos force-pushed the rust-backend branch 4 times, most recently from c9abff8 to 24706b4 Compare January 30, 2024 17:52
@T-Dynamos
Copy link
Contributor Author

@misl6 Done!, After review I will document all.

@misl6
Copy link
Member

misl6 commented Mar 10, 2024

@T-Dynamos can you please rebase on top of develop as CI pipeline for apple-silicon-m1 looks to fail on an issue that has already been fixed via #2842

@T-Dynamos T-Dynamos force-pushed the rust-backend branch 3 times, most recently from 7c812e9 to ef829d5 Compare March 17, 2024 07:59
@T-Dynamos
Copy link
Contributor Author

@misl6 CI looks happy now.

@T-Dynamos T-Dynamos requested a review from misl6 March 18, 2024 00:21
@misl6
Copy link
Member

misl6 commented Mar 19, 2024

Looks like we have a failure on 3 jobs with almost the same issue:

2024-03-17T08:12:12.1384103Z     sources = self.generate_sources(sources, (lib_name, build_info))
2024-03-17T08:12:12.1384888Z               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2024-03-17T08:12:12.1386664Z   File "/home/user/.local/share/python-for-android/build/other_builds/numpy/arm64-v8a__ndk_target_21/numpy/numpy/distutils/command/build_src.py", line 378, in generate_sources
2024-03-17T08:12:12.1388177Z     source = func(extension, build_dir)
2024-03-17T08:12:12.1388716Z              ^^^^^^^^^^^^^^^^^^^^^^^^^^
2024-03-17T08:12:12.1390355Z   File "/home/user/.local/share/python-for-android/build/other_builds/numpy/arm64-v8a__ndk_target_21/numpy/numpy/core/setup.py", line 702, in get_mathlib_info
2024-03-17T08:12:12.1391864Z     config_cmd.compiler = bk_c.cxx_compiler()
2024-03-17T08:12:12.1392413Z                           ^^^^^^^^^^^^^^^^^
2024-03-17T08:12:12.1393495Z AttributeError: 'UnixCCompiler' object has no attribute 'cxx_compiler'

Can you make sure you have rebased on top of latest develop branch?

I'm wondering if the setuptools version bump may be the issue?

@T-Dynamos
Copy link
Contributor Author

Looks like we have a failure on 3 jobs with almost the same issue:

2024-03-17T08:12:12.1384103Z     sources = self.generate_sources(sources, (lib_name, build_info))
2024-03-17T08:12:12.1384888Z               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2024-03-17T08:12:12.1386664Z   File "/home/user/.local/share/python-for-android/build/other_builds/numpy/arm64-v8a__ndk_target_21/numpy/numpy/distutils/command/build_src.py", line 378, in generate_sources
2024-03-17T08:12:12.1388177Z     source = func(extension, build_dir)
2024-03-17T08:12:12.1388716Z              ^^^^^^^^^^^^^^^^^^^^^^^^^^
2024-03-17T08:12:12.1390355Z   File "/home/user/.local/share/python-for-android/build/other_builds/numpy/arm64-v8a__ndk_target_21/numpy/numpy/core/setup.py", line 702, in get_mathlib_info
2024-03-17T08:12:12.1391864Z     config_cmd.compiler = bk_c.cxx_compiler()
2024-03-17T08:12:12.1392413Z                           ^^^^^^^^^^^^^^^^^
2024-03-17T08:12:12.1393495Z AttributeError: 'UnixCCompiler' object has no attribute 'cxx_compiler'

Can you make sure you have rebased on top of latest develop branch?

I'm wondering if the setuptools version bump may be the issue?

Yes setuptools version bump is the issue, I think this is the time to update numpy to latest version. I will do this ASAP.

@T-Dynamos T-Dynamos force-pushed the rust-backend branch 3 times, most recently from b129f20 to 26a5415 Compare March 20, 2024 08:44
@T-Dynamos
Copy link
Contributor Author

@misl6 Done! 🔥 .

@T-Dynamos
Copy link
Contributor Author

@misl6
Any update on this?

Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

I've added a comment regarding a specific change.

No need to make changes, but I only need an explanation ATM.

Regarding the numpy fix: Hacky, but it works, and I'm fine with it.

Hopefully PEP738 will help us to clean python-for-android from these hacks and perform isolated builds as 1,2,3 ... (so let's keep an eye on the progress).

pythonforandroid/recipes/cffi/__init__.py Outdated Show resolved Hide resolved
@T-Dynamos T-Dynamos force-pushed the rust-backend branch 3 times, most recently from ed61368 to ec74c5f Compare March 31, 2024 00:50
@T-Dynamos T-Dynamos requested a review from misl6 March 31, 2024 05:05
@T-Dynamos
Copy link
Contributor Author

@misl6 Fixed.

misl6
misl6 previously approved these changes Mar 31, 2024
Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

Let's wait for the CI/CD pipeline to complete, but LGTM

@T-Dynamos
Copy link
Contributor Author

Sorry had to do one last change, please cancel other actions.

Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

Let's wait to the CI/CD pipeline to complete, but LGTM.

@misl6 misl6 merged commit 8110faf into kivy:develop Mar 31, 2024
33 checks passed
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.

building pydantic failed
5 participants