Fixes Setup.py error on Windows #676 #734
Conversation
Fixes Setup,py error on Windows, added shell script to check python environment, print the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Sai-Adarsh If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@micw523 gave a PR. could you check this out. |
|
/assign @roycaihw |
|
/assign @micw523 |
|
@Sai-Adarsh: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@appendly: changing LGTM is restricted to assignees, and only kubernetes-client/python repo collaborators may be assigned issues. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
scripts/setup-fix.bat
Outdated
| echo Python environment found.. | ||
| fi | ||
| python --version | ||
| cp -R ../kubernetes/base/* ../kubernetes/ |
There was a problem hiding this comment.
what's the reason to do the 'cp -R' above?
There was a problem hiding this comment.
what's the reason to do the 'cp -R' above?
"When we clone submodule with git the folders in the /kubernetes/base is supposed to be symbolically linked in the /kubernetes directory. In Windows this is not done. For a Windows developer, we’ll need to have a script copying all the base/* folders into */. "
^ is what @micw523 requested in his issue. so 'cp -R' does the job.
scripts/setup-fix.bat
Outdated
| @@ -0,0 +1,11 @@ | |||
| if ! [ -x "$(command -v python)" ]; | |||
| then | |||
| echo Python environment notfound.. | |||
There was a problem hiding this comment.
probably add double quotes around the echo string
scripts/setup-fix.bat
Outdated
| then | ||
| echo Python environment notfound.. | ||
| else | ||
| echo Python environment found.. |
There was a problem hiding this comment.
ditto
cool. will do the necessary changes
There was a problem hiding this comment.
| echo Python environment found.. | |
| echo Python environment found. |
scripts/setup-fix.bat
Outdated
| echo "Python environment notfound.." | ||
| else | ||
| echo "Python environment found.." | ||
| fi |
There was a problem hiding this comment.
Please check windows batch scripting syntax for if
|
To be honest this looks like bash instead of a Windows bat. You could probably use it in a Cygwin but I think Windows developers would prefer a genuine bat. |
|
@micw523 done with the changes. could you check on the new commit :) |
done with the changes. |
scripts/setup-fix.bat
Outdated
| @@ -0,0 +1,5 @@ | |||
| ( where python >null | |||
| echo Python environment found.. | |||
There was a problem hiding this comment.
In this case Python environment found will be printed regardless.
There was a problem hiding this comment.
python > nullandfound.
cool. done with the changes.
done. |
done with changes. |
|
/lgtm |
scripts/setup-fix.bat
Outdated
| @@ -0,0 +1,6 @@ | |||
| ( python --version>nul 2>&1 && ( | |||
| echo Python environment found. | |||
There was a problem hiding this comment.
just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
There was a problem hiding this comment.
just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
works fine without double qoute, so thought of reverting the change :)
cool. |
|
New changes are detected. LGTM label has been removed. |
@roycaihw could you check on the new changes? |
|
Could you add the comments to the beginning of the file? Basically I was asking for a description so that people can tell what it's for and when it should be used. I think we don't need inline comments. Something like "Install python client in Windows. Windows doesn't have symbolic link that this repo uses. This batch script provides a workaround for symbolic link by copying the folders over." Also please squash your commits before submit /hold cancel |
|
Will lg()tm after squash |
cool. |
Would like to join a GitHub team of kubernetes-client, and highly interested to join as a reviewer. Have previously merged a fix (kubernetes-client#734).
Fixes Setup.py error on Windows, added shell script to check python environment, prints the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install issue #676