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

fix: revive hangs on Windows if go.mod is not found #1123

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

alexandear
Copy link
Contributor

The PR fixes #1120.

The problem is the retrieveModFile function, which hangs forever when go.mod is not found. filepath.Dir returns C:/ on Windows, but the implementation expects "." or "/".

The bug appeared after changing path.Dir to filepath.Dir in #1073. We can revert to using path.Dir, but a better solution is to add strings.TrimPrefix(dir, filepath.VolumeName(dir)) for Windows.

@alexandear alexandear changed the title Fix hanging revive on Windows when without go.mod Fix hanging revive on Windows when run without go.mod Nov 14, 2024
@alexandear alexandear changed the title Fix hanging revive on Windows when run without go.mod fix: revive hangs on Windows when run without go.mod Nov 14, 2024
@alexandear alexandear changed the title fix: revive hangs on Windows when run without go.mod fix: revive hangs on Windows when if go.mod is not found Nov 14, 2024
@alexandear alexandear force-pushed the fix-hang-on-windows branch 2 times, most recently from f6dc5d4 to 1012b14 Compare November 14, 2024 13:23
@alexandear alexandear changed the title fix: revive hangs on Windows when if go.mod is not found fix: revive hangs on Windows if go.mod is not found Nov 14, 2024
@chavacava
Copy link
Collaborator

Hi @alexandear, thanks for investigating the problem and proposing a fix.

About: We can revert to using path.Dir, but a better solution is to add strings.TrimPrefix(dir, filepath.VolumeName(dir)) for Windows.

Why do you prefer adding OS-dependent code instead of reverting to path.Dir?

@alexandear
Copy link
Contributor Author

Hi @alexandear, thanks for investigating the problem and proposing a fix.

About: We can revert to using path.Dir, but a better solution is to add strings.TrimPrefix(dir, filepath.VolumeName(dir)) for Windows.

Why do you prefer adding OS-dependent code instead of reverting to path.Dir?

  1. path/filepath is preferable for working with cross-platform paths. This is by design of the standard package.
  2. Changing to filepath fixed a bug with different behavior of retrieveModFile on Unix and Windows.

For example, add fmt.Println("retrieveModFile dir: ", dir) inside the for loop in retrieveModFile.

And run revive on Mac:

❯ ./revive/revive t.go
retrieveModFile dir:  /Users/Oleksandr_Redko/src/github.com/mgechev
retrieveModFile dir:  /Users/Oleksandr_Redko/src/github.com
retrieveModFile dir:  /Users/Oleksandr_Redko/src
retrieveModFile dir:  /Users/Oleksandr_Redko
retrieveModFile dir:  /Users
retrieveModFile dir:  /
t.go:1:1: invalid file t.go: t.go:1:1: expected 'package', found 'func'

Run revive on Mac:

PS C:\Users\Oleksandr_Redko\src\github.com\mgechev> revive.exe t.go
retrieveModFile dir:  C:\Users\Oleksandr_Redko\src\github.com\mgechev
retrieveModFile dir:  .
t.go:1:1: invalid file t.go: t.go:1:1: expected 'package', found 'func'

As we see, retrieveModFile searches for the go.mod on Mac inside 6 directories, and on Windows only inside 1 directory.

With filepath.Dir and the fix in this PR:

PS C:\Users\Oleksandr_Redko\src\github.com\mgechev> revive.exe t.go
retrieveModFile dir:  C:\Users\Oleksandr_Redko\src\github.com\mgechev
retrieveModFile dir:  \Users\Oleksandr_Redko\src\github.com
retrieveModFile dir:  \Users\Oleksandr_Redko\src
retrieveModFile dir:  \Users\Oleksandr_Redko
retrieveModFile dir:  \Users
retrieveModFile dir:  \
t.go:1:1: invalid file t.go: t.go:1:1: expected 'package', found 'func'

Actually, I refactored the fix and removed if runtime.GOOS == "windows", please recheck.

@alexandear alexandear force-pushed the fix-hang-on-windows branch 3 times, most recently from dd0f892 to 226fd36 Compare November 14, 2024 17:18
@alexandear
Copy link
Contributor Author

GitHub Actions workflow for running tests on Windows added in #1125

@chavacava chavacava merged commit 74e2417 into mgechev:master Nov 15, 2024
4 checks passed
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.

Revive 1.5.0 process hangs
2 participants