-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
[open62541] Fix flakiness/bugginess #7607
Conversation
- We used to `get-pip` on all non-Linux systems; we should be downloading and running it only on Windows. - `get-pip`'s download link was volatile, and the SHA would change. We now download it from a versioned link, which should not change As part of these, we bumped the number from 0.30.0-2 to 0.30.0-3
c46034c
to
4d41be8
Compare
The design idea, which surprisingly was approved since it seems now it is not, was to download pip on any system, if it was not available and only in case also easy_install was not found I did something identical also in the It’s a pity anyway that we are losing this functionality, I was hoping to integrate it as a vcpkg function to use it also in other ports! |
) | ||
execute_process(COMMAND ${PYTHON3_DIR}/python${EXECUTABLE_SUFFIX} ${PYTHON3_DIR}/get-pip.py) | ||
if(CMAKE_HOST_WIN32) | ||
# Must not modify system copy of python3 -- on CMAKE_HOST_WIN32, we have our own private copy |
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.
We are not messing with system copy of python3, we are just adding a package in the user folder... no need to have sudo privilege, nothing...
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.
cc @ras0219-msft, why did you do it this way? (this is copied directly from #7382, so I don't know why the decision was made)
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.
While you're absolutely right to point out that requiring sudo would also be a reason we shouldn't do this, in this case it's because, as a rule, vcpkg will not modify your system outside its specifically allowed directories (the local vcpkg instance and a single config dir like $HOME/vcpkg).
It would be a great future improvement if we could either:
- Check for the existence of the package ahead of time in the top message and give a better error
- Have a private copy of python that we can muck with as much as we like
@ras0219-msft could you comment here since it appears that the change was actually a part of one of your PRs. The stated rationale seemed fine to me. |
get-pip
on all non-Linux systems; we should bedownloading and running it only on Windows.
get-pip
's download link was volatile, and the SHA would change. Wenow download it from a versioned link, which should not change
As part of these, we bumped the number from 0.30.0-2 to 0.30.0-3