Skip to content

mingw: Add platform checking of GoDocBrowser for Cygwin on windows#3611

Open
rwxguo wants to merge 12 commits into
fatih:masterfrom
rwxguo:doc-browser-gitbash
Open

mingw: Add platform checking of GoDocBrowser for Cygwin on windows#3611
rwxguo wants to merge 12 commits into
fatih:masterfrom
rwxguo:doc-browser-gitbash

Conversation

@rwxguo

@rwxguo rwxguo commented Dec 15, 2023

Copy link
Copy Markdown
Contributor

@rwxguo rwxguo closed this Dec 15, 2023
@rwxguo rwxguo reopened this Dec 15, 2023
@bhcleek

bhcleek commented Dec 15, 2023

Copy link
Copy Markdown
Collaborator

Thank you

@bhcleek bhcleek added this to the vim-go v1.29 milestone Dec 15, 2023
@rwxguo

rwxguo commented Dec 19, 2023

Copy link
Copy Markdown
Contributor Author

welcome, happy holiday !

@bhcleek bhcleek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for the redraw and that check for cygwin. I'd like to see some changes to the cygwin detection, though.

In addition to the suggested edit I provided, can you add go#util#IsCygwin thusly:

function! IsCygwin()
  return has('win32unix`)
endfunction

Unfortunately, I don't have a windows machine to test with. Can you make the suggested edits and test to make sure they're correct?

I'm not sure vim-go should support mingw in the way that this PR and your others enable, because I think think it's possible that with mingw there's other use cases that would break with the changes here and elsewhere. I'm going to have to give this and your other PRs some thought. For now, can you remove the mingw detection in this PR?

Comment thread autoload/go/config.vim Outdated
@rwxguo

rwxguo commented Dec 24, 2023

Copy link
Copy Markdown
Contributor Author

@bhcleek Thank you for your guide and instruction. I added cygwin detection go#util#IsCygwin.

The reason using uname to check the platform is inspired by the maven-wrapper project, which is widely used in enterprise level java projects using SpringBoot etc.

Plus, I also leverage the vim built-in feature list has('win32unix') to make it more concise.

I have tested these factors on three major cygwin platforms, the result is for your reference:

platform has('win32') has('win64') has('win32unix') uname
Cygwin 0 0 1 CYGWIN_...
GitBash (64bit) 0 0 1 MINGW64_...
MSYS2 - mingw32.exe 0 0 1 MINGW32_...
MSYS2 - mingw64.exe 0 0 1 MINGW64_...
MSYS2 - msys2.exe 0 0 1 MSYS_...

@rwxguo

rwxguo commented Dec 24, 2023

Copy link
Copy Markdown
Contributor Author

By the way, I found there is a util function go#util#IsUsingCygwinShell which might not correct, because it uses go#util#IsWin, but based on my above testing result, go#util#IsWin always return 0 on all cygwin-like platform...

 " Checks if using:
 " 1) Windows system,
 " 2) And has cygpath executable,
 " 3) And uses *sh* as 'shell'
function! go#util#IsUsingCygwinShell()
  return go#util#IsWin() && executable('cygpath') && &shell =~ '.*sh.*'
endfunction

The go#util#IsUsingCygwinShell is used in path.vim, for your information

@bhcleek

bhcleek commented Dec 24, 2023

Copy link
Copy Markdown
Collaborator

Yes, go#util#IsUsingCygwinShell is for a different kind of use case than you've been working on. The last I checked, it is correct for the use case for which it is intended.

Comment thread autoload/go/util.vim Outdated
@rwxguo rwxguo reopened this Dec 25, 2023
@rwxguo

rwxguo commented Dec 25, 2023

Copy link
Copy Markdown
Contributor Author

As CYGWIN uses cygstart instead of start, but MSYS2 and GitBash uses start, I made the cygwin checking as a seprated "elseif", and use rundll32 instead of start rundll32 or cygwin rundll32. (treat cygwin as a separated system, rather than a windows variation)

@bhcleek bhcleek modified the milestones: vim-go v1.29, vim-go v1.30 Apr 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants