-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat: adopt maturin #616
feat: adopt maturin #616
Conversation
Signed-off-by: Keming <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.
Files not reviewed (3)
- MANIFEST.in: Language not supported
- Makefile: Language not supported
- requirements/dev.txt: Language not supported
Comments suppressed due to low confidence (1)
mosec/runtime.py:273
- Ensure that the 'mosec' binary is correctly located in the PATH to avoid runtime errors. Previously, the path was explicitly set using importlib_files.
self.process = subprocess.Popen([str("mosec"), config_path])
Signed-off-by: Keming <[email protected]>
Signed-off-by: Keming <[email protected]>
Signed-off-by: Keming <[email protected]>
I have verified that this works for:
|
There are many ways to install Python packages, but if they're managed by modern tools, I think they should work. One common issue from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 6 out of 14 changed files in this pull request and generated no comments.
Files not reviewed (8)
- MANIFEST.in: Language not supported
- Makefile: Language not supported
- requirements/dev.txt: Language not supported
- setup.py: Evaluated as low risk
- tests/test_service.py: Evaluated as low risk
- mosec/init.py: Evaluated as low risk
- Cargo.toml: Evaluated as low risk
- .github/workflows/nightly.yml: Evaluated as low risk
Comments suppressed due to low confidence (1)
mosec/runtime.py:273
- Using a hardcoded string for the executable path might cause issues if the executable is not in the system's PATH. Consider using a resolved path instead.
self.process = subprocess.Popen([str("mosec"), config_path])
I have a minor concern with this. Here the python / pip refers to /usr/bin/python? |
Not exactly. It's installed by |
I see, thanks for clarification. Since almost all commonly used installation methods work well, let's merge this nice update to make the build simpler! |
bin
instead ofpyo3
binding, so it has nothing to do with the ABI3benefits
defects
mosec
for both rust/py packages, this will add amosec
bin to the scripts, so people may accidentally executemosec xxx
and get an error (since it's not meant to be called directly by the users)Cargo.toml