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

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

rwxguo
Copy link
Contributor

@rwxguo rwxguo commented Dec 15, 2023

@rwxguo rwxguo closed this Dec 15, 2023
@rwxguo rwxguo reopened this Dec 15, 2023
@bhcleek
Copy link
Collaborator

bhcleek commented Dec 15, 2023

Thank you

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

rwxguo commented Dec 19, 2023

welcome, happy holiday !

Copy link
Collaborator

@bhcleek bhcleek left a comment

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?

autoload/go/config.vim Outdated Show resolved Hide resolved
@rwxguo
Copy link
Contributor Author

rwxguo commented Dec 24, 2023

@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
Copy link
Contributor Author

rwxguo commented Dec 24, 2023

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
Copy link
Collaborator

bhcleek commented Dec 24, 2023

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.

autoload/go/util.vim Outdated Show resolved Hide resolved
@rwxguo rwxguo reopened this Dec 25, 2023
@rwxguo
Copy link
Contributor Author

rwxguo commented Dec 25, 2023

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)

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.

None yet

2 participants