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

Create python tutorial. #1648

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

gogo2464
Copy link

I am going to reuse my template for the tutorial.

@gogo2464 gogo2464 force-pushed the document-python2 branch 4 times, most recently from 00bcf45 to 122a1e1 Compare July 10, 2023 12:01
@gogo2464 gogo2464 marked this pull request as ready for review July 10, 2023 12:02
@gogo2464 gogo2464 requested a review from a team as a code owner July 10, 2023 12:02
@gogo2464 gogo2464 requested review from bendk and removed request for a team July 10, 2023 12:02
@gogo2464
Copy link
Author

The minimal example compiles. I tested it in my system. Do you would like cicd to test it?

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

This is looking in reasonable shape, thanks! Many of the files are missing newline characters at the end of the file, and we probably should not assume the reader is running Windows, but I'm not quite sure what we should do there yet. It would also be great if we can work out how to set setup.py to be the smallest possible example - I think there's a fair bit there that isn't strictly necessary and it's already complex enough that anything which isn't necessary is likely to confuse the reader.

docs/manual/src/python/setup.md Outdated Show resolved Hide resolved
docs/manual/src/python/setup.md Outdated Show resolved Hide resolved
@@ -0,0 +1,23 @@
[package]
Copy link
Member

Choose a reason for hiding this comment

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

I think the directory name could be just "python"


# Building

.\run.bat
Copy link
Member

Choose a reason for hiding this comment

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

This is Windows specific so should probably be removed

return "10.7"


# The logic for specifying wheel tags in setuptools/wheel is very complex, hard
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this complexity for this sample - I think it's fine if the target only runs on the Python version it was built with.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. This looks like the horrible hack we did over in Glean. And the proper way and IMO what we should recommend is using something like maturin (e.g. see mozilla/glean#2345).
Though for a small example I'm fine with a minimal setup.py still.

Copy link
Author

Choose a reason for hiding this comment

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

I must watch your PR but it is a draft.

Copy link
Author

@gogo2464 gogo2464 Jul 11, 2023

Choose a reason for hiding this comment

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

@badboy I watched your incomming PR. It seems also very much better with .pytoml. @mhammond can I keep the current work as legacy or should I immediately rewrite it please?

Copy link
Author

Choose a reason for hiding this comment

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

at this moment I work with maturin.

Copy link
Member

Choose a reason for hiding this comment

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

I think @badboy and I are both saying that for this trivial example, there's no need to use maturin. It might make good sense for your project, but for the purposes of this example, we should aim for it being as small as reasonably possible.

@gogo2464
Copy link
Author

I agree. This example is long. I might remove class build(_build): content. But then I will need to run 2 commands from batch or from bash.

docs/manual/src/python/setup.md Outdated Show resolved Hide resolved
docs/manual/src/python/setup.md Outdated Show resolved Hide resolved

[dependencies]
uniffi = {version = "*", features = [ "cli" ]}
uniffi_macros = "*"
Copy link
Member

Choose a reason for hiding this comment

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

These all seem odd - you probably want to do what the other examples do, and I don't think this example needs a direct dependency on uniffi_macros or on uniffi_bindgen

@gogo2464 gogo2464 force-pushed the document-python2 branch 9 times, most recently from 079470c to a78acae Compare July 11, 2023 17:40
@gogo2464
Copy link
Author

for a weird reason the tesing fails without uniffi_macro = "*"

@gogo2464
Copy link
Author

It does not fails with uniffi_macro. Let me try without and with a rebase please.

@gogo2464
Copy link
Author

Yes I maintain that the cicd does not pass over with not uniffi_macro.

@badboy
Copy link
Member

badboy commented Jul 12, 2023

Yes I maintain that the cicd does not pass over with not uniffi_macro.

Then that's an issue we should address. We're not gonna recommend star-dependencies in official examples; example should showcase best practices.

@gogo2464
Copy link
Author

I have already reported. I have not found why it still works with not uniffi_macro for some people. #1645

@badboy
Copy link
Member

badboy commented Jul 17, 2023

Just as a note: I'll try to help pushing this forward tomorrow.

Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

I appreciate the effort, but after going through this I think it's easier if I take this over and prepare a minimal example we can land.

Comment on lines +22 to +23
python3 ./src/setup.py bdist_wheel --verbose ;
pip3 install ./dist/* --force-reinstall ;
Copy link
Member

Choose a reason for hiding this comment

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

No need for semicolons

Suggested change
python3 ./src/setup.py bdist_wheel --verbose ;
pip3 install ./dist/* --force-reinstall ;
python3 ./src/setup.py bdist_wheel --verbose
pip3 install ./dist/* --force-reinstall

Copy link
Member

Choose a reason for hiding this comment

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

oh fwiw, I think we should just go ahead and recommend python3 -m build instead of now deprecated ways.

@@ -19,6 +19,7 @@ members = [
"examples/sprites",
"examples/todolist",
"examples/traits",
"examples/distributing/uniffi-rust-to-python-library",
Copy link
Member

Choose a reason for hiding this comment

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

Why the subfolder?

Copy link
Author

Choose a reason for hiding this comment

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

Somebody told me to create a distributing subfolder.

docs/manual/src/SUMMARY.md Show resolved Hide resolved
docs/manual/src/python/setup.md Show resolved Hide resolved
@gogo2464
Copy link
Author

I appreciate the effort, but after going through this I think it's easier if I take this over and prepare a minimal example we can land.

yes sure. Then I will update.

@gogo2464
Copy link
Author

Hello. I am confused. Is it expected to wait for the new python template .pytoml in order to document it or can I continue with setup.py?

@badboy
Copy link
Member

badboy commented Aug 11, 2023

Hey, sorry. I didn't yet have time to work on this. As I said I will take this over and prepare the docs in a way we want to land them. There's currently no more work for you to be done.

@gogo2464
Copy link
Author

Alright. Do I document the old way with a deprecated mention in the doc?

@gogo2464
Copy link
Author

or I just wait at least.

@gogo2464
Copy link
Author

Hey, sorry. I didn't yet have time to work on this. As I said I will take this over and prepare the docs in a way we want to land them. There's currently no more work for you to be done.

alright. I spent some days to code. It could be fine if I can merge something. Tell me if it is not necessary please?

uniffi = { version = "*", features = [ "build", "cli" ] }

[[bin]]
# This can be whatever name makes sense for your project, but the rest of this tutorial assumes uniffi-bindgen.
Copy link

Choose a reason for hiding this comment

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

As a noob, this is pretty confusing because it makes it ambiguous to whether the intention is to reference this uniffi-bindgen or the uniffi binary tool uniffi-bindgen

@gogo2464
Copy link
Author

Hello I read the page https://mozilla.github.io/uniffi-rs/python/configuration.html . Is this issue fixed? We may could set my way for an optional python tutorial on the old setup.py?

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