-
-
Notifications
You must be signed in to change notification settings - Fork 121
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
xcaddy does not download newly published module versions, even when specified #221
Comments
What's the command you ran and the output? I'll try to reproduce it. |
AFAICT the issue can only be reproduced once per release. Once it's cached in the go module proxy then the problem goes away. So to reproduce it you have to make a brand new release of an existing caddy module and try to xcaddy build with it, as I describe in the reproduction steps in my initial comment. |
You can see the effect in this series of GH action runs: https://github.com/infogulch/xtemplate/actions/runs/12643197444 You can find the xcaddy logs for each run in the root of the Obviously the code for each run was not changed. The only thing that changed between the failed runs and the successful run is that I ran Note the timings:
|
This is strange because xcaddy isn't doing anything special. It's a wrapper around Lines 182 to 193 in eb894be
xcaddy cannot know to fail or complain if the |
Thanks; from the logs:
That's weird. It then downloads caddy/v2, but it shouldn't be getting v1 at all... I would check your plugins for correctness here... Mohammed is right though; xcaddy literally just wraps the (Closing, since I don't think there's anything we can fix in xcaddy for this, but feel free to continue discussion!) |
So I checked the logs and xcaddy never runs
|
What does your script execute? Why the replace?
That's why it isn't picking up the new version. You're telling Go to use the local source code, regardless of the specified version. Just run this:
|
The full command that I execute is |
This is what I'm saying. Your command doesn't specify a version. Add the |
Ok I'm not doing a very good job describing the problem. Just now I ran this which references
But it built with v0.2.2, completely ignoring the version specifier, and it never executed
Do you agree that it should fail, and that it should try to run Next I ran this, which is the same as above except it doesn't override the
This fails as expected with the error
So maybe the issue is that if one module has a directory override, then it ignores version specifiers for other named modules? |
Oh, ok, I see it now. When you had the local override, the next module was skipped for some reason. I'm thinking this is an unintentional bug, one of those infamous off-by-one error. Let's check it. |
Thank you for your patience! FYI I also tried changing the order of the arguments but it didn't make any difference. |
I figured the culprit line. Here xcaddy loops over all plugins/dependencies to execute Lines 181 to 196 in 3339110
Critically, in this segment xcaddy skips replaced modules by checking if the package path of this iteration is prefixed by a package path that is replaced, i.e. if the current package path is Lines 183 to 191 in 3339110
The assumption made during the write of the tool is written in the comment. Our assumption is apparently not optimal. I don't have an immediate solution from the top of my head right now. If you (or any passerby) figures it out, feel free to share. I can think of a few ugly workarounds... |
Ah, haha thanks Mohammed. We might be better off checking for prefix match plus "/" or EOS. I've done this a few times before but this code was written before I was so wise 🙃 |
Wow I really hit an edge case here. If I understand correctly the conditions are:
Then B's version specifier is ignored because it is assumed to be a subpackage of A that has a local override. I think Matt's "prefix plus /" solution should work: if strings.HasPrefix(p.PackagePath+"/", repl) { But does it makes sense to allow this scenario in the first place? It seems like ignoring a version specifier for one package because its parent package has a local override would be fragile. I guess it could make a mess to change this behavior now. Valid use cases are already for testing/integration purposes where you want to be able to override modules. It's probably fine. |
Just tried; it doesn't. The package path |
Oh my mistake, the args are (string to check, prefix) not the reverse. Maybe this?: if strings.HasPrefix(p.PackagePath, repl+"/") { |
I published a new version of my module by tagging it and pushing the tag to GH, and immediately tried to xcaddy build with it like
xcaddy build --with github.com/infogulch/[email protected]
, but xcaddy continued to use the old module version. After I manually updated the go proxy service withgo get github.com/infogulch/[email protected]
then it pulled in the new version. (See https://proxy.golang.org/#faq-new-version)Some issues:
xcaddy build --with module@version
to download new module versions likego get module@version
does.I expect this to reproduce with the following steps:
git tag -a v0.0.1; git push --tags
go get [email protected]
xcaddy build --with [email protected]
git commit ...; git tag -a v0.0.2; git push --tags
go get
xcaddy build --with [email protected]
v0.0.1
The text was updated successfully, but these errors were encountered: