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

Can't do module replacement with github forks #50

Closed
francislavoie opened this issue Feb 21, 2021 · 13 comments · Fixed by #51
Closed

Can't do module replacement with github forks #50

francislavoie opened this issue Feb 21, 2021 · 13 comments · Fixed by #51
Assignees

Comments

@francislavoie
Copy link
Member

francislavoie commented Feb 21, 2021

So I want to have a replacement like:

replace github.com/mholt/caddy-dynamicdns => github.com/francislavoie/caddy-dynamicdns caddyfile

So I try this:

xcaddy build --with github.com/mholt/caddy-dynamicdns=github.com/francislavoie/caddy-dynamicdns@caddyfile

But it does this:

...
2021/02/21 07:25:03 [INFO] exec (timeout=0s): /usr/local/go/bin/go get -d -v github.com/mholt/caddy-dynamicdns=github.com/francislavoie/caddy-dynamicdns@caddyfile
go get github.com/mholt/caddy-dynamicdns=github.com/francislavoie/caddy-dynamicdns@caddyfile: malformed module path "github.com/mholt/caddy-dynamicdns=github.com/francislavoie/caddy-dynamicdns": invalid char '='

This obviously isn't quite right, cause it never did the go mod edit -replace.

Next I tried without @caddyfile (my branch), and I get this:

2021/02/21 07:29:24 [INFO] Resolved relative replacement github.com/mholt/caddy-dynamicdns=github.com/francislavoie/caddy-dynamicdns to /root/github.com/francislavoie/caddy-dynamicdns
...
2021/02/21 07:29:24 [INFO] Replace github.com/mholt/caddy-dynamicdns => /root/github.com/francislavoie/caddy-dynamicdns
2021/02/21 07:29:24 [INFO] exec (timeout=10s): /usr/local/go/bin/go mod edit -replace github.com/mholt/caddy-dynamicdns=/root/github.com/francislavoie/caddy-dynamicdns
...
caddy imports
        github.com/mholt/caddy-dynamicdns: github.com/mholt/caddy-dynamicdns@v0.0.0-00010101000000-000000000000: replacement directory /root/github.com/francislavoie/caddy-dynamicdns does not exist

So xcaddy thinks that path is relative but it's not, it's a git repo.

I think to detect if it's relative, we should look if there's a . in front, so if you want to refer to something in the current dir then you do ./blah in the replacement. That should allow github module paths to work.

The workaround obviously for now is to git clone the relevant repo then use a relative path on that, but it would be nice so that people can easily test branches/PRs for plugins without cloning.

@mholt
Copy link
Member

mholt commented Feb 22, 2021

Hmm, wouldn't you just change the require dependency from github.com/mholt/caddy-dynamicdns to github.com/francislavoie/caddy-dynamicdns?

@francislavoie
Copy link
Member Author

No because that would imply the master branch, but if it's not on the master branch that won't work.

@mholt
Copy link
Member

mholt commented Feb 22, 2021

Set the version of that require directive to another branch then?

@francislavoie
Copy link
Member Author

francislavoie commented Feb 22, 2021

What do you mean? What would the command look like?

@mholt
Copy link
Member

mholt commented Feb 22, 2021

require directives consist of a module name and a version:

github.com/mholt/acmez v0.1.3

so instead of a version on the master branch, choose a different branch name.

@francislavoie
Copy link
Member Author

francislavoie commented Feb 22, 2021

Right but you would do that with @ in the xcaddy command, but that doesn't work. Did you miss that in the post above (2nd code block)?

@mholt
Copy link
Member

mholt commented Feb 22, 2021

I mean do this instead:

xcaddy build --with github.com/francislavoie/caddy-dynamicdns@caddyfile

@francislavoie
Copy link
Member Author

That won't work because the module in that repo is github.com/mholt/caddy-dynamicdns so a replace needs to happen.

@mholt
Copy link
Member

mholt commented Feb 22, 2021

Oh... I see... how does the go tool determine which kind of replacement to do, I wonder? (Can look into it later)

@francislavoie
Copy link
Member Author

francislavoie commented Feb 22, 2021

It does it just fine if you specify go mod edit -replace github.com/mholt/caddy-dynamicdns=/root/github.com/francislavoie/caddy-dynamicdns@caddyfile but the problem is our "relative path" resolve step breaks it at the xcaddy CLI layer. (this is all explained above)

@mohammed90
Copy link
Member

FWIW, I started working on it last night but didn't finish it. This is where I'm at now.

Diff:

diff --git a/cmd/xcaddy/main.go b/cmd/xcaddy/main.go
index 001f462..39d8b72 100644
--- a/cmd/xcaddy/main.go
+++ b/cmd/xcaddy/main.go
@@ -76,7 +76,7 @@ func runBuild(ctx context.Context, args []string) error {
 			})
 			if repl != "" {
 				// adjust relative replacements in current working directory since our temporary module is in a different directory
-				if !filepath.IsAbs(repl) {
+				if strings.HasPrefix(repl, ".") && !filepath.IsAbs(repl) {
 					repl, err = filepath.Abs(repl)
 					if err != nil {
 						log.Fatalf("[FATAL] %v", err)
@@ -288,7 +288,9 @@ func trapSignals(ctx context.Context, cancel context.CancelFunc) {
 func splitWith(arg string) (module, version, replace string, err error) {
 	const versionSplit, replaceSplit = "@", "="
 
-	parts := strings.SplitN(arg, versionSplit, 2)
+	modules := strings.SplitN(arg, replaceSplit, 2)
+
+	parts := strings.SplitN(modules[0], versionSplit, 2)
 	module = parts[0]
 
 	if len(parts) == 1 {

Test run:

=== RUN   TestSplitWith
    caddyserver/xcaddy/cmd/xcaddy/main_test.go:72: Test 2: Expected module 'replace' but got '' (input='module@version=replace')
    caddyserver/xcaddy/cmd/xcaddy/main_test.go:72: Test 3: Expected module 'replace' but got '' (input='module=replace')

@francislavoie
Copy link
Member Author

It also needs to support module=replace@version

@mohammed90
Copy link
Member

It also needs to support module=replace@version

Hence

but didn't finish it

😄

mohammed90 added a commit that referenced this issue Feb 26, 2021
mohammed90 added a commit that referenced this issue Feb 26, 2021
* accept replacements with branches of github forks

Closes #50

* remove `!filepath.IsAbs(repl)` when checking path of replacement

If the replacement path starts with a `.`, then the path is definitely not absolute.
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 a pull request may close this issue.

3 participants