-
Notifications
You must be signed in to change notification settings - Fork 328
feat: Add Uv Python package manager #10020
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
base: main
Are you sure you want to change the base?
Conversation
plugins/package-managers/python/src/funTest/kotlin/UvFunTest.kt
Dismissed
Show dismissed
Hide dismissed
eca7fa1
to
61dff26
Compare
helper-cli/src/main/kotlin/commands/repoconfig/GenerateScopeExcludesCommand.kt
Fixed
Show fixed
Hide fixed
61dff26
to
f9c0758
Compare
27a370a
to
8e01f73
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10020 +/- ##
============================================
- Coverage 69.59% 69.55% -0.04%
Complexity 1462 1462
============================================
Files 270 270
Lines 9665 9690 +25
Branches 1025 1034 +9
============================================
+ Hits 6726 6740 +14
- Misses 2489 2495 +6
- Partials 450 455 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
45a10e7
to
2638532
Compare
1b134d5
to
ed7dd63
Compare
11ffddc
to
736c767
Compare
Signed-off-by: Helio Chissini de Castro <helio.chissini.de.castro@cariad.technology>
736c767
to
3789735
Compare
plugins/package-managers/python/src/test/kotlin/utils/PythonUtilsTest.kt
Fixed
Show fixed
Hide fixed
Signed-off-by: Helio Chissini de Castro <helio.chissini.de.castro@cariad.technology>
3789735
to
5a34f28
Compare
Just as a somewhat related remark, Dependabot now can also handle uv. |
Right on time, i say |
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.
I believe this needs major rework for work without python-inspector
, which is know to have several problems.
Please also remove the merge commit from the history.
@@ -147,6 +147,7 @@ ARG PYTHON_PIPENV_VERSION | |||
ARG PYTHON_POETRY_VERSION | |||
ARG PYTHON_POETRY_PLUGIN_EXPORT_VERSION | |||
ARG PYTHON_SETUPTOOLS_VERSION | |||
ARG PYTHON_UV_VERSION |
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.
As this is the first commit introducing uv, the commit should briefly explain what it is, plus contain a link to https://docs.astral.sh/uv/.
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.
Spelling-wise, it seems we should use "uv" instead of "Uv".
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.
Also, the commit message should make explicit that this is done as a preparation for adding uv support as a package manager in ORT.
@@ -25,6 +25,7 @@ ARG PYTHON_POETRY_VERSION=2.0.1 | |||
ARG PYTHON_POETRY_PLUGIN_EXPORT_VERSION=1.9.0 | |||
ARG PYTHON_SETUPTOOLS_VERSION=74.1.3 | |||
ARG PYTHON_VERSION=3.11.10 | |||
ARG PYTHON_UV_VERSION=0.6.5 |
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.
By now, there's https://github.com/astral-sh/uv/releases/tag/0.6.6.
import org.ossreviewtoolkit.utils.test.matchExpectedResult | ||
|
||
class UvFunTest : WordSpec({ | ||
"Python 3" should { |
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.
"Python 3" -> "Uv"
internal object UvCommand : CommandLineTool { | ||
override fun command(workingDir: File?) = "uv" | ||
|
||
override fun transformVersion(output: String) = output.substringAfter("version ").removeSuffix(")") |
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.
For me, the version output is
$ uv --version
uv 0.6.2
So why the .substringAfter("version ").removeSuffix(")")
? Shouldn't it be just .substringAfter("uv ")
?
*/ | ||
@OrtPlugin( | ||
displayName = "Uv", | ||
description = "An extremely fast Python package and project manager.", |
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.
IMO we should not adopt to their marketing language here, and drop "extremely fast". So just say "A Python package and project manager".
description = "An extremely fast Python package and project manager.", | ||
factory = PackageManagerFactory::class | ||
) | ||
class Uv(override val descriptor: PluginDescriptor = UvFactory.descriptor, private val config: PipConfig) : |
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.
I believe the only reason why Pip.kt
, Pipenv.kt
and Poetry.kt
shared PipConfig
so far is that all of these are handled by python-inspector
(and PipConfig
contains python-inspector
-specific config).
But for the independent Uv
implementation, I believe we should have a separate UvConfig
.
internal const val PYPROJECT_FILENAME = "pyproject.toml" | ||
} | ||
|
||
override val globsForDefinitionFiles = listOf("uv.lock") |
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.
This line triggered cc7059a on my side. Once that PR is merged, please take over the final wording of that comment here.
.distinctBy { it.id } | ||
.toSet() |
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.
This also looks unnecessary. As I realize you've copied this from the Poetry
implementation, I'm removing that as part of #10051 as well.
} | ||
|
||
/** | ||
* Return the result of running Python inspector against a requirements file generated by exporting the dependencies |
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.
Oh, so you are using python-inspector
after all... not that falling back to using requirements.txt
files should really just be a last resort, as those files only contain a flat list of direct dependencies without much metadata.
I was under the assumption that a modern manager like uv
offers better ways to query dependencies.
So I guess we'd want astral-sh/uv#4711 to be implemented. |
Uv is Astral.sh Python package/project manager solution. The backend is based on Rust and is a merge result of two projects, the original package manager uv and the Python project manager rye,
Reference: