Skip to content
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

PR: Fix bootstrap.py git subprocess call on Windows #19867

Merged
merged 2 commits into from
Oct 17, 2022

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Oct 17, 2022

Description of Changes

Missing Windows case to call git from subprocess from bootstrap.py

Issue(s) Resolved

Fixes #19865

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: dalthviz

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @dalthviz!

@mrclary
Copy link
Contributor

mrclary commented Oct 17, 2022

@dalthviz @ccordoba12, will subprocess.run(["git", ...]) find git.cmd as well as git.exe on Windows?

I know that spyder.utils.programs.find_git has a lot of robust checks, but maybe we don't need to worry about that for bootstrap.py since this is primarily for developers who should have git installed properly on their system. So maybe "git" will work well enough for all platforms in this situation?

@ccordoba12
Copy link
Member

ccordoba12 commented Oct 17, 2022

will subprocess.run(["git", ...]) find git.cmd as well as git.exe on Windows?

Good point. I think it should work as long as shell=True on Windows.

@mrclary
Copy link
Contributor

mrclary commented Oct 17, 2022

git.exe was found for me with shell=False

@dalthviz
Copy link
Member Author

Thanks for the feedback @mrclary ! Checking I was able to change it to just git and without shell=True. Should I change this to just remove the "/usr/bin/env" so this will end up being for all the platforms:

        result = subprocess.run(
            ["git", "merge-base", "--fork-point", "master"]
        )

@ccordoba12
Copy link
Member

+1 from me because it'd make the code simpler.

@mrclary
Copy link
Contributor

mrclary commented Oct 17, 2022

+1 from me too.
Just the caveats that I mentioned: there may be instances where git is not installed, in which case the subprocess will fail and not master will always be written to boot_branch.txt and spyder will always be installed. Since this is primarily for developers, I think this edge case will be very rare.

bootstrap.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 merged commit d53ff3f into spyder-ide:5.x Oct 17, 2022
ccordoba12 added a commit that referenced this pull request Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants