Skip to content

fix: _termui_impl.open_url() — 'start' on Windows is a cmd built-in, not an executable#3186

Merged
AndreasBackx merged 2 commits into
pallets:mainfrom
karpierz:fixes_for_windows
Apr 29, 2026
Merged

fix: _termui_impl.open_url() — 'start' on Windows is a cmd built-in, not an executable#3186
AndreasBackx merged 2 commits into
pallets:mainfrom
karpierz:fixes_for_windows

Conversation

@karpierz
Copy link
Copy Markdown
Contributor

@karpierz karpierz commented Jan 15, 2026

fix: _termui_impl.open_url() — 'start' on Windows is a cmd built-in command, not an executable

@karpierz karpierz mentioned this pull request Jan 27, 2026
Closed
@davidism davidism linked an issue Jan 27, 2026 that may be closed by this pull request
click.launch() does not work for (non local) urls on Windows #3164
Closed
@kdeldycke kdeldycke changed the base branch from main to stable April 16, 2026 09:34
@kdeldycke kdeldycke changed the title fix: _termui_impl.open_url() — 'start' on Windows is a cmd built-in, not an executable fix: _termui_impl.open_url() — 'start' on Windows is a cmd built-in, not an executable Apr 21, 2026
@kdeldycke kdeldycke added bug windows Issues pertaining to the Windows environment security Pull requests that address a security vulnerability labels Apr 21, 2026
@kdeldycke
Copy link
Copy Markdown
Collaborator

kdeldycke commented Apr 21, 2026

shell=True has been eliminated entirely in Click code base with #3245. So there is no reason to re-introduce it. Can you give use more details about the reason we need this?

@kdeldycke kdeldycke added this to the 8.4.0 milestone Apr 21, 2026
@kdeldycke
Copy link
Copy Markdown
Collaborator

kdeldycke commented Apr 21, 2026 •
edited
Loading

OK got it. Click code is broken on Windows because start is a cmd.exe built-in, not a standalone executable. So without a shell=True the subprocess.call will fail. So that's a legitimate bug report.

But there is a better fix. Instead of re-introducing shell=True, we can use os.startfile().

@kdeldycke
Copy link
Copy Markdown
Collaborator

kdeldycke commented Apr 21, 2026

I just re-implemented the fix. This PR is ready to be merged in the next devlopment cycle of Click.

@Rowlando13 Rowlando13 changed the base branch from stable to main April 29, 2026 07:02
@Rowlando13
Copy link
Copy Markdown
Collaborator

Rowlando13 commented Apr 29, 2026

I will fix changelog then it's ready to merge.

@kdeldycke @karpierz
Use os.startfile() on Windows instead of the start built-in
76472b1 
Closes #3164

Co-Authored-By: Adam Karpierz <akarpierz@gmail.com>
@kdeldycke
Copy link
Copy Markdown
Collaborator

kdeldycke commented Apr 29, 2026

I will fix changelog then it's ready to merge.

I just fixed the merge conflicts and rebased on top of main. This PR is ready.

@davidism
Copy link
Copy Markdown
Member

davidism commented Apr 29, 2026 •
edited
Loading

Check that this still works on cygwin. I know we don't officially support it, but I think some of this code being changed was there in part due to someone wanting to use it from cygwin or msys2 maybe?

Never mind, missed the literal next line of the diff.

@AndreasBackx AndreasBackx merged commit c5cced7 into pallets:main Apr 29, 2026
12 checks passed
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 14, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@davidism davidism davidism approved these changes

@Rowlando13 Rowlando13 Awaiting requested review from Rowlando13

Assignees

No one assigned

Labels

bug security Pull requests that address a security vulnerability windows Issues pertaining to the Windows environment

Projects

None yet

Milestone

8.4.0

Development

Successfully merging this pull request may close these issues.

click.launch() does not work for (non local) urls on Windows

5 participants

@karpierz @kdeldycke @Rowlando13 @davidism @AndreasBackx