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 main.go generation when executing on a windows machine #32

Merged
merged 4 commits into from
Sep 25, 2020

Conversation

jaysonsantos
Copy link
Contributor

@jaysonsantos jaysonsantos commented Aug 23, 2020

When normalizing the importPath it will try to write on main.go files with imports
like github.com/caddyserver/xcaddy/c:\xcaddy

The fix is basically changing from path.Join(currentModule, strings.TrimPrefix(cwd, filepath.ToSlash(moduleDir))) to path.Join(currentModule, filepath.ToSlash(strings.TrimPrefix(cwd, moduleDir)))

When normalizing the `importPath` it will try to write on `main.go` files with imports
like `github.com/caddyserver/xcaddy/c:\xcaddy`
@jaysonsantos
Copy link
Contributor Author

I just rebased it

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! How does it handle the situation where you're in a subdir of a module, i.e. cwd = /xcaddy/subdir, and on Windows, i.e. c:\xcaddy\subdir?

@@ -71,3 +71,34 @@ func TestSplitWith(t *testing.T) {
}
}
}

func Test_normalizeImportPath(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func Test_normalizeImportPath(t *testing.T) {
func TestNormalizeImportPath(t *testing.T) {

@@ -243,6 +243,10 @@ func runDev(ctx context.Context, args []string) error {
return cmd.Wait()
}

func normalizeImportPath(currentModule string, cwd string, moduleDir string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func normalizeImportPath(currentModule string, cwd string, moduleDir string) string {
func normalizeImportPath(currentModule, cwd, moduleDir string) string {

@jaysonsantos
Copy link
Contributor Author

Hey @mholt
Do you mean that this testcase:

args{
	currentModule: "github.com/caddyserver/xcaddy",
	cwd:           "c:\\xcaddy\\subdir",
	moduleDir:     "c:\\xcaddy\\subdir",
}

should be:
github.com/caddyserver/xcaddy/subdir
?

@mholt
Copy link
Member

mholt commented Aug 31, 2020

I mean we need to add at least 2 more test cases.

@jaysonsantos
Copy link
Contributor Author

If I understood correctly, something like this?

diff --git a/cmd/xcaddy/main_test.go b/cmd/xcaddy/main_test.go
index af9f0de..23975c4 100644
--- a/cmd/xcaddy/main_test.go
+++ b/cmd/xcaddy/main_test.go
@@ -88,11 +88,21 @@ func TestNormalizeImportPath(t *testing.T) {
 			cwd:           "/xcaddy",
 			moduleDir:     "/xcaddy",
 		}, "github.com/caddyserver/xcaddy"},
+		{"linux-subpath", args{
+			currentModule: "github.com/caddyserver/xcaddy",
+			cwd:           "/xcaddy/subdir",
+			moduleDir:     "/xcaddy",
+		}, "github.com/caddyserver/xcaddy/subdir"},
 		{"windows-path", args{
 			currentModule: "github.com/caddyserver/xcaddy",
 			cwd:           "c:\\xcaddy",
 			moduleDir:     "c:\\xcaddy",
 		}, "github.com/caddyserver/xcaddy"},
+		{"windows-subpath", args{
+			currentModule: "github.com/caddyserver/xcaddy",
+			cwd:           "c:\\xcaddy\\subdir",
+			moduleDir:     "c:\\xcaddy",
+		}, "github.com/caddyserver/xcaddy/subdir"},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

@mholt
Copy link
Member

mholt commented Aug 31, 2020

Yes I think that's right! (My brain's all over the place today but see if that works)

@jaysonsantos
Copy link
Contributor Author

Thank you!
Done

@jaysonsantos
Copy link
Contributor Author

Hmm, and it fails on Linux for windows subpaths.
Should I split up the test for linux and windows and run windows' one only on it?

@mholt
Copy link
Member

mholt commented Aug 31, 2020

Well, Windows paths shouldn't come up on Unix at all, so running tests for Windows path on Unix does seem a bit unnecessary.

Or, maybe something like this?

	return path.Join(currentModule, filepath.ToSlash(strings.TrimPrefix(cwd, filepath.ToSlash(moduleDir))))

@jaysonsantos
Copy link
Contributor Author

jaysonsantos commented Sep 1, 2020

I guess for that to work I need to make the cwd slashed as well.
I will try in a few and commit if it works

@mholt
Copy link
Member

mholt commented Sep 1, 2020

I guess for that to work I need to make the cwd slashed as well.

Oh, yes, that's probably true!

@jaysonsantos
Copy link
Contributor Author

Hey there ToSlash takes the separator from the OS so running it on windows paths won't work so I changed the tests to run the windows cases only only when GOOS matches

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Cool, that works. Thanks so much for contributing this!

@mholt mholt merged commit 4f3e0ef into caddyserver:master Sep 25, 2020
@jaysonsantos jaysonsantos deleted the windows-import-path-fix branch September 26, 2020 12:12
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