-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Python: fix a bug in the Makefile for checking if uv
is installed or not
#9763
base: main
Are you sure you want to change the base?
Conversation
uv
exists or notuv
exists or not
python/Makefile
Outdated
@@ -37,7 +37,7 @@ install: | |||
UV_VERSION = $(shell uv --version 2> /dev/null) | |||
install-uv: | |||
# Check if uv is installed | |||
ifdef UV_VERSION | |||
ifeq ($(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.
doesn't this mean that if UV_VERSION is defined and not empty, the flow will enter the "else" block, print "uv could not be found" etc?
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.
Hey @dluc , my bad, I have updated the ifeq
to ifneq
and tested the Makefile locally.
I was in the process of testing the script, and I marked the PR as draft
, and therefore I didn't expect to trigger a formal review.
Anyways, please let me know if the new Makefile
looks good to you. I also updated the description of this PR to include more information.
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.
@dluc do you have further comments on this PR?
uv
exists or notuv
uv
uv
is installed or not
Motivation and Context
Why is this change required?
This is the issue: Python: Bug: Make Install does not work on clean builds #9067
A user reports that the Makefile cannot install
uv
.What problem does it solve?
What scenario does it contribute to?
With this fix, users who do not have
uv
installed in their machine will now have the Makefile to installuv
for them.If it fixes an open issue, please link to the issue here.
Python: Bug: Make Install does not work on clean builds #9067
Description
In the current Makefile, we assign the evaluation result of
$(shell uv --version 2> /dev/null)
toUV_VERSION
, which will be an empty string""
. If we useifdef UV_VERSION
to check whetheruv
is installed or not, it will always be true. Therefore, I useifneq ($(UV_VERSION),)
to check the existenceuv
.Another minor improvement that I made to the
Makefile
is that I guard the.SILENT
keyword withifndef
so that users can turn on/off theVERBOSE
mode easily. For example, they can enableVERBOSE
mode by runningmake install VERBOSE=1
. By default, it uses theSILENT
mode.Contribution Checklist