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

@karpierz karpierz commented Jan 15, 2026

Copy link
Copy Markdown
Contributor

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 Windows-specific behavior labels Apr 21, 2026
@kdeldycke

kdeldycke commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator

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

kdeldycke commented Apr 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

kdeldycke commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator

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

Rowlando13 commented Apr 29, 2026

Copy link
Copy Markdown
Member

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

kdeldycke commented Apr 29, 2026

Copy link
Copy Markdown
Collaborator

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

davidism commented Apr 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

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

windows Windows-specific behavior

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