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

Handle replacements properly for runDev() #69

Merged
merged 8 commits into from
Sep 1, 2021

Conversation

simnalamburt
Copy link
Contributor

@simnalamburt simnalamburt commented Aug 29, 2021

Closes #68

  1. Handle all possible replacements properly for runDev(). Replacement targets are not always paths.
  2. Parse replacement info from go list -m -json all. It's more robust in this way.

I have tested all possible cases with replacements like below. So hopefully replacements won't cause any issue further at least for the runDev().

replace (
	replacetest1 v1.2.3 => golang.org/x/example v0.0.0-20210811190340-787a929d5a0d
	replacetest2 => golang.org/x/example v0.0.0-20210407023211-09c3a5e06b5d
	replacetest3 v1.2.3 => ./fork1
	replacetest4 => ./fork2
	replacetest5 v1.2.3 => /home/username/fork3
	replacetest6 => /home/username/fork4
)

Use forked version of xcaddy if you want to test this PR:

go install github.com/simnalamburt/xcaddy/cmd/xcaddy@466a1a4b86

References

@mohammed90
Copy link
Member

Thank you, @simnalamburt! If you don't mind, since you've stumbled upon an edge case, do you mind adding few tests to further validate the logic? We'd hate to break existing users while fixing a bug.

@simnalamburt
Copy link
Contributor Author

simnalamburt commented Aug 29, 2021 via email

@mholt
Copy link
Member

mholt commented Aug 30, 2021

I think unit tests would go a long way, at least. Might have to split some of the logic into a separate function to do that effectively. Basically checking that the input and outputs are as expected. https://pkg.go.dev/testing is your friend. 👍

It's more robust in this way
1.  Handle all possible replacements properly for runDev()

    Replacement targets are not always paths.

    Reference:
      https://pkg.go.dev/cmd/go/internal/list#pkg-variables

2.  Parse replacement info from 'go list -m -json all'

    It's more robust in this way
@simnalamburt
Copy link
Contributor Author

simnalamburt commented Aug 30, 2021

Maybe I can separate executing go list -m -json all part and parsing the output part. Then unit testing the parsing part should be easy since it's IO free. Thanks @mholt! :)

@simnalamburt
Copy link
Contributor Author

I think I’ve done some basic testing :) Please feel free to take a look!

@simnalamburt
Copy link
Contributor Author

Omg the test was UNIX specific 🤦 hang on

@mohammed90
Copy link
Member

Omg the test was UNIX specific 🤦 hang on

Yay for tests 🙂

`go list -m -json all` contains all informations we need so we don't
need extra `go list -m -json` call.
@mohammed90
Copy link
Member

Omg the test was UNIX specific 🤦 hang on

Let me try to make the issue clearer. Note the slashes here:

Expected Got
[
	{
		replacetest1@v1.2.3
		golang.org/x/example@v0.0.0-20210811190340-787a929d5a0d
	}
	{
		replacetest2@v0.0.1 
		golang.org/x/example@v0.0.0-20210407023211-09c3a5e06b5d
	} 
	{
		replacetest3@v1.2.3 
		\home\work\module\fork1
	} 
	{
		github.com/simnalamburt/module 
		\home\work\module
	} 
	{
		replacetest4@v0.0.1 
		\srv\fork2
	} 
	{
		replacetest5@v1.2.3 
		\home\work\module\fork3
	}
]
[
	{
		replacetest1@v1.2.3 
		golang.org/x/example@v0.0.0-20210811190340-787a929d5a0d
	}
	{
		replacetest2@v0.0.1 
		golang.org/x/example@v0.0.0-20210407023211-09c3a5e06b5d
	}
	{
		replacetest3@v1.2.3 
		\home\work\module\fork1
	}
	{
		github.com/simnalamburt/module 
		/home/work/module
	}
	{
		replacetest4@v0.0.1 
		\home\work\module\srv\fork2
	}
	{
		replacetest5@v1.2.3 
		\home\work\module\fork3
	}
]

@simnalamburt
Copy link
Contributor Author

OK I think I fixed windows build, but now I failed Go 1.16 build due to the use of //go:build style build constraints.

@mohammed90
Copy link
Member

OK I think I fixed windows build, but now I failed Go 1.16 build due to the use of //go:build style build constraints.

For this you have to use the older style // +build windows just below the newer style. Have them both in the same file just below each other.

@simnalamburt
Copy link
Contributor Author

Note: I tried not to split main_unix_test.go and main_windows_test.go, but their JSON looks too different so it was really hard to generate two JSON in one test code.

@francislavoie
Copy link
Member

If only Go supported the ternary condition operator 😂

Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

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

This looks good! One minor nitpick then it's all right.

cmd/main.go Outdated Show resolved Hide resolved
@simnalamburt
Copy link
Contributor Author

I changed map[string]interface{} into Module and the CI has passed in my fork.

Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

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

Had to handle one last minor nitpick. Perfecto. Thanks @simnalamburt!

@mohammed90 mohammed90 merged commit 3d8622d into caddyserver:master Sep 1, 2021
@simnalamburt simnalamburt deleted the issue-68 branch September 1, 2021 18:33
mholt added a commit that referenced this pull request Sep 28, 2022
Follow-up to #69

I had a situation where caddy/v2 v2.4.6 was in my go.mod and a replace
on it to use local caddy repo.

When I ran `xcaddy run` to test a caddy module, it failed to run because
the replace included the version v2.4.6, even though somehow the buildenv
go.mod had v2.6.1 (the latest), and the replace directive specified
@v2.4.6 only, so the replacement didn't happen.

I don't think versions in the replacement source/origin are
really that useful for xcaddy.

If experience proves this wrong, we can figure out a better solution.
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.

xcaddy development mode fails if 'go.mod' includes 'replace' directive
4 participants