You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "messense (via GitHub)" <gi...@apache.org> on 2023/03/22 03:08:51 UTC
[GitHub] [incubator-opendal] messense opened a new pull request, #1729: feat(bindings/python): convert to mixed Python/Rust project layout
messense opened a new pull request, #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729
Use `_opendal` to indicate that it's an implementation detail.
Closes #1728
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] messense commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144192059
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
It's the common practice in Python ecosystem. For example the std `ssl` module is backed by a `_ssl` extension module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] messense commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144193275
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
Sure, but nobody links to `_opendal`.
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
Sure, but nobody links to `_opendal` or `_ssl`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144193297
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
A `_opendal` python module looks good to me, and it's good to follow the common practice in Python ecosystem. But the so name is really not good...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] messense commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144192059
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
It's the common practice in Python ecosystem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo merged pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] messense commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144196390
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
The final so name in `whl` is actually a little more complicated than you think...
```
Archive: dist/opendal-0.30.3-cp311-cp311-macosx_11_0_arm64.whl
Length Date Time Name
--------- ---------- ----- ----
1740 03-22-2023 03:04 opendal-0.30.3.dist-info/METADATA
106 03-22-2023 03:04 opendal-0.30.3.dist-info/WHEEL
2918 03-22-2023 03:04 opendal/__init__.pyi
81 03-22-2023 03:04 opendal/__init__.py
49968299 03-22-2023 03:04 opendal/_opendal.cpython-311-darwin.so
460 03-22-2023 03:04 opendal-0.30.3.dist-info/RECORD
--------- -------
49973604 6 files
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144198337
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
Hi, could you please provide some comments to clarify why we chose `_opendal` over `opendal_python` or `_opendal_python`? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144197930
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
Ok, let's just use `_opendal`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] messense commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144257410
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
FYI, https://github.com/PyO3/maturin/pull/1526 will allow us to just remove `name = "_opendal"` from `[lib]`, as it's already specified in `[package.metadata.maturin]`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144195309
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
> It's just that in Python anything with `_` prefix is considered private, so `opendal_python` would be considered public API which isn't what we want.
I understand your point. However, I am concerned about the current setup generating a file named `lib_opendal.so`, which does not seem appropriate to me.
If we have to live with `lib_opendal.so`, I prefer to use `libopendal.so` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144192946
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
> For example the std `ssl` module is backed by a `_ssl` extension module.
But they link to `libssl.so` instead of `_ssl.so`, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] messense commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144197355
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
It's fine, just mouthful when you are already in a Python package.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144191665
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
I think `_opendal` looks worse. Would it be better to use `opendal_python` instead?
I know it's weird to see `opendal.opendal_python`, but `_opendal.so` seems even weirder to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] messense commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144194105
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
It's just that in Python anything with `_` prefix is considered private, so `opendal_python` would be considered public API which isn't what we want.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144196676
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
Is it a good idea to use `_opendal_python`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1729: feat(bindings/python): convert to mixed Python/Rust project layout
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1729:
URL: https://github.com/apache/incubator-opendal/pull/1729#discussion_r1144293161
##########
bindings/python/Cargo.toml:
##########
@@ -28,7 +28,7 @@ version = "0.30.3"
[lib]
crate-type = ["cdylib"]
doc = false
-name = "opendal"
+name = "_opendal"
Review Comment:
That's great!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org