Conversation
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
… zf/fix-py Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
zFernand0
changed the title
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
… zf/fix-py Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
… zf/fix-py Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
|
… zf/fix-py Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
|
Server artifacts: |
… the main branch Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
t1m0thyj
left a comment
t1m0thyj
left a comment
There was a problem hiding this comment.
Thanks for your work on this Fernando! Left a few comments, will review again after manual testing.
traeok
left a comment
traeok
left a comment
There was a problem hiding this comment.
Changes make sense to me, I have the same suggestions as Timothy so I just gave them a thumbs up 😋 will approve once those are resolved
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
🐍 Precompiled Python bindingsBuilt from 📦 Download Hosted as an asset on the |
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
… zf/fix-py Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
|
traeok
left a comment
traeok
left a comment
There was a problem hiding this comment.
Thanks for working on this Fernando - leaving request for changes due to the missing sys import and the Windows allowlist paths, but the changes LGTM otherwise!
| print(f"Packaged Python wrapper: {filename}") | ||
| else: | ||
| print(f"Error: Python wrapper not found: {filename}") | ||
| sys.exit(1) |
There was a problem hiding this comment.
sys is never imported, this may surface a NameError. Consider adding import sys to the top of this file
| print(f"Copied object file: {filename}") | ||
| else: | ||
| print(f"Error: precompiled object file not found: {filename}") | ||
| sys.exit(1) |
There was a problem hiding this comment.
Similar suggestion as above for package_precompiled.py - missing an import sys at the top
| function resolveExecutable(name: string): string | null { | ||
| const trustedDirs = | ||
| process.platform === "win32" | ||
| ? [path.join(process.env.SystemRoot ?? "C:\\Windows", "System32")] |
There was a problem hiding this comment.
On Windows, both the GitHub CLI and Git for Windows are installed in Program Files, not System32 - this detection likely won't catch gh or git when run on Windows clients.
I'd recommend adding these Program Files paths to the trustedDirs allowlist:
X:\Program Files\GitX:\Program Files\GitHub CLI
Note that I used X: because the system drive may not be C: for all clients. I think we should use process.env.SystemDrive when applicable (instead of assuming C:) if we plan to have an allowlist for Windows paths
There was a problem hiding this comment.
In the context of a dev tool, I don't know how important it is to guard against untrusted dirs for executables? Thinking this could be fragile if someone has an unconventional environment 😋
There was a problem hiding this comment.
That's a fair point, although even if this was a production app and in a typical environment where Git / GitHub CLI is installed on Windows, this logic has no effect. It's essentially dead code / redundant logic on win32 in its current state. So maybe we should just remove it entirely?
There was a problem hiding this comment.
If SWIG is absent and no precompiled bundle has been posted, the workflow emits ::warning:: and skips the Python test step rather than failing. Do we plan to resolve this in a separate PR? I noticed the TODO around fixing automated tests so I wasn't sure if we wanted to address that here.
5 participants
Supported Functionality
What It Does
Build SWIG extendersGenerate SWIG wrappersBuild Python extensionbuilding '_zusf_py' extensionbuilding '_zds_py' extensionbuilding '_zjb_py' extension_EXTand_OPEN_SYS_FILE_EXT=1) tosetup.py:fldata_tand__amrc_typeundefined)package_zbind.py>zbind_src_dist.tar.gz)package_precompiled.py>zbind_bin_dist.tar.gz)README.mdtonative/python/bindings/detailing installation procedures for both distributionsHow to Test
npm run z:upload pythonnpm run z:python:buildnpm run z:python:pack(downloads thedist/zbind_bin_dist.tar.gz)npm run z:python:post -- <PR-NUMBER>(e.g.npm run z:python:post -- 968)Review Checklist
I certify that I have:
Additional Comments
TODO:
npm run z:python:test