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

support go.mod replace version pinning #27

Merged
merged 3 commits into from
Aug 10, 2020
Merged

Conversation

jpughcs
Copy link
Contributor

@jpughcs jpughcs commented Aug 6, 2020

Fix for error caused by replace directives with version pinning:

2020/08/06 10:53:59 [INFO] Temporary folder: /go/src/x/y/service/cta-proxy/buildenv_2020-08-06-1053.131880924
2020/08/06 10:53:59 [INFO] Writing main module: /go/src/x/y/service/cta-proxy/buildenv_2020-08-06-1053.131880924/main.go
2020/08/06 10:53:59 [INFO] Initializing Go module
2020/08/06 10:53:59 [INFO] exec (timeout=10s): /usr/local/bin/go mod init caddy 
go: creating new go.mod: module caddy
2020/08/06 10:53:59 [INFO] Replace x/y => /go/src/x/y
2020/08/06 10:53:59 [INFO] exec (timeout=10s): /usr/local/bin/go mod edit -replace x/y=/go/src/x/y 
2020/08/06 10:53:59 [INFO] Replace github.com/apache/thrift => github.com/apache/thrift v0.0.0-20190309152529-a9b748bb0e02
2020/08/06 10:53:59 [INFO] exec (timeout=10s): /usr/local/bin/go mod edit -replace github.com/apache/thrift=github.com/apache/thrift v0.0.0-20190309152529-a9b748bb0e02 
go mod: -replace=github.com/apache/thrift=github.com/apache/thrift v0.0.0-20190309152529-a9b748bb0e02: invalid new path: malformed import path "github.com/apache/thrift v0.0.0-20190309152529-a9b748bb0e02": invalid char ' '
2020/08/06 10:53:59 [ERROR] exit status 1

@mholt
Copy link
Member

mholt commented Aug 6, 2020

Thanks, but I don't think I can accept this as-is. I'm not even aware of any problems in xcaddy right now.

What is the problem? You'll have to motivate the reason for this change.

@jpughcs
Copy link
Contributor Author

jpughcs commented Aug 7, 2020

My apologies, I should have created a formal issue first...

Expected Behavior

Using a replace directive in go.mod with a versioned pinned ModulePath should correctly execute go mod edit -replace with the given replacement during buildenv initialization.

go.mod:

replace github.com/apache/thrift => github.com/apache/thrift v0.0.0-20190309152529-a9b748bb0e02

buildenv initialization:

[INFO] exec (timeout=10s): /usr/local/bin/go mod edit -replace github.com/apache/thrift=github.com/apache/thrift@v0.0.0-20190309152529-a9b748bb0e02

Current Behavior

go.mod:

replace github.com/apache/thrift => github.com/apache/thrift v0.0.0-20190309152529-a9b748bb0e02

buildenv initialization:

[INFO] Replace github.com/apache/thrift => github.com/apache/thrift v0.0.0-20190309152529-a9b748bb0e02
[INFO] exec (timeout=10s): /usr/local/bin/go mod edit -replace github.com/apache/thrift=github.com/apache/thrift v0.0.0-20190309152529-a9b748bb0e02 
go mod: -replace=github.com/apache/thrift=github.com/apache/thrift v0.0.0-20190309152529-a9b748bb0e02: invalid new path: malformed import path "github.com/apache/thrift v0.0.0-20190309152529-a9b748bb0e02": invalid char ' '

Note the space in the replacement directive between the new ModulePath and Version. xcaddy currently builds the mod edit -replace command from these ModulePaths verbatim. This is incompatible with the mod edit -replace command as it requires there to be an @ symbol between the ModulePath and Version.

Steps to Reproduce

  1. Add a version pinned replacement directive to go.mod
  2. Run an xcaddy command that initializes a buildenv

@mholt
Copy link
Member

mholt commented Aug 7, 2020

What is your xcaddy command though? Like, how can I reproduce the problem? I use --replace all the time and it works fine.

@jpughcs
Copy link
Contributor Author

jpughcs commented Aug 7, 2020

I am running simply xcaddy and xcaddy list-modules. The output in my first comment, yesterday, was from running xcaddy list-modules.

@jpughcs
Copy link
Contributor Author

jpughcs commented Aug 7, 2020

This is what xcaddy is trying to run with my replace directive in go.mod:

go mod edit -replace 'github.com/apache/thrift=github.com/apache/thrift v0.0.0-20190309152529-a9b748bb0e02'

If you run that manually, you will see the same error invalid char ' '

However,

go mod edit -replace 'github.com/apache/thrift=github.com/apache/thrift@v0.0.0-20190309152529-a9b748bb0e02'

... works, but there is nothing currently in xcaddy, that I can tell, which reformats the components of the replace directive like this.

@mholt
Copy link
Member

mholt commented Aug 7, 2020

I see. I usually use replace for local copies, like:

replace github.com/caddyserver/certmagic => ../certmagic

And so there is no version string.

What is the use case for replace with a version string, rather than simply using go get (i.e. changing the version in go.mod's require statement)?

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.

Elegant fix though, thanks for the test cases too!

builder.go Outdated
Comment on lines 150 to 153
if r == "" {
return ""
}
return regexp.MustCompile(`\sv`).ReplaceAllString(string(r), "@v")
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's any need for regular expressions, and we can't assume the version starts with v (it could be a commit or branch name):

Suggested change
if r == "" {
return ""
}
return regexp.MustCompile(`\sv`).ReplaceAllString(string(r), "@v")
return strings.Replace(string(r), " ", "@", 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I understand, the "version" here will always start with a v. See: https://golang.org/ref/mod#versions

Copy link
Member

@mholt mholt Aug 7, 2020

Choose a reason for hiding this comment

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

We can try it, but go get accepts commits and branches too.

Make this change anyway (with a v if you want) and I can merge this.

@jpughcs
Copy link
Contributor Author

jpughcs commented Aug 7, 2020

What is the use case for replace with a version string, rather than simply using go get (i.e. changing the version in go.mod's require statement)?

See:
uber-go/cadence-client#523
uber-go/cadence-client#812

Quite an annoying problem...

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, thanks for the fix! This works for you then?

@jpughcs
Copy link
Contributor Author

jpughcs commented Aug 10, 2020

Yes this works. Thank you! 😃

@mholt mholt merged commit b7fd102 into caddyserver:master Aug 10, 2020
@mholt
Copy link
Member

mholt commented Aug 10, 2020

Okay, thanks again! (I just pushed a couple minor tweaks.) I appreciate the contribution!

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