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

Add --replace for dependency replacement #186

Merged
merged 3 commits into from
May 29, 2024

Conversation

WeidiDeng
Copy link
Member

@WeidiDeng WeidiDeng commented May 29, 2024

Officially --with is used to add caddy modules to a caddy build. It can be used to change dependencies for a caddy build, i.e. change caddy core to a fork, use quic-go fork.

However, some dependencies can't be replaced this way, for example golang.org/x/net can't be replaced this way.

For dependencies this introduces a new grammar that a ~ prefix to a with to mean it's a dependency instead of a module.

I can't think why someone would want to name their plugin with a ~, is it even allowed by golang? It will break caddy builds for those unfortunate souls.

Add --with option to replace dependency.

Also fixed when replacing, module version is lost. According to go documents, If the @v in old@v is omitted, a replacement without a version on the left side is added, which applies to all versions of the old module path.

@mholt
Copy link
Member

mholt commented May 29, 2024

Interesting!

How would you feel if we simply made a new flag called --replace that does what --with does, but without the added import? It strictly adds a replace line to the go.mod.

I think the ~ syntax is a little wonky :)

@WeidiDeng WeidiDeng changed the title use ~ prefix for dependency replacement add --with for dependency replacement May 29, 2024
@francislavoie francislavoie changed the title add --with for dependency replacement Add --replace for dependency replacement May 29, 2024
@francislavoie
Copy link
Member

I think it could be refactored a bit to deduplicate the code, but it's short enough to not really be a problem.

Please also update the readme with the new option

Comment on lines +102 to 110
You can even replace Caddy core using the `--replace` flag:

```
$ xcaddy build \
--with github.com/caddyserver/caddy/v2=../../my-caddy-fork
--replace github.com/caddyserver/caddy/v2=../../my-caddy-fork

$ xcaddy build \
--with github.com/caddyserver/caddy/v2=github.com/my-user/caddy/v2@some-branch
--replace github.com/caddyserver/caddy/v2=github.com/my-user/caddy/v2@some-branch
```
Copy link
Member

Choose a reason for hiding this comment

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

Well, technically it still works with --with cause we check for that. I think here it should show an example replacing a dep like x/net as you're doing, it's a better example.

@@ -73,6 +74,7 @@ $ xcaddy build [<caddy_version>]

- `--output` changes the output file.
- `--with` can be used multiple times to add plugins by specifying the Go module name and optionally its version, similar to `go get`. Module name is required, but specific version and/or local replacement are optional.
- `--replace` can be used multiple times to replace dependencies to specific forks or local replacements. Useful in development environment to fix bugs in dependencies.
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
- `--replace` can be used multiple times to replace dependencies to specific forks or local replacements. Useful in development environment to fix bugs in dependencies.
- `--replace` can be used multiple times to replace dependencies to specific forks or local replacements. Useful in development environment when trying to fix bugs in Caddy's dependencies, via a fork or local checkout.

Copy link
Member

Choose a reason for hiding this comment

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

What about "--replace is like --with, but does not add a blank import to the code; it only writes a replace directive to go.mod, which is useful when developing on Caddy's dependencies that aren't Caddy modules." ?

@francislavoie francislavoie added the enhancement New feature or request label May 29, 2024
@mholt
Copy link
Member

mholt commented May 29, 2024

This LGTM, but I agree with the readme suggestions to make things a little clearer. I'll merge this so we can at least tag a release with it since the code apparently works.

@mholt mholt merged commit d7277db into caddyserver:master May 29, 2024
8 checks passed
francislavoie added a commit that referenced this pull request May 29, 2024
mholt pushed a commit that referenced this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants