-
Notifications
You must be signed in to change notification settings - Fork 160
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
build: force old timer behavior #4620
Conversation
Until golang/go#69312 is resolved, force the old timer behavior by specifying an older go version in the go.mod file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for figuring it out.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not really solved yet.
I guess bazel does not respect the go.mod as it is not really aware of it :sad-panda:
We probably want to set the env var to disable the timer change. I will take a look eventually
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another stop where the go version must go.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my old PR to set go to 1.23: https://github.com/scionproto/scion/pull/4618/files#diff-5493ff8e9397811510e780de47c57abb70137f1afe85d1519130dc3679d60ce5
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I don't really want to revert the go version update and would rather just disable the semantic change.
Latest commit achieves that and looks promising on CI: https://buildkite.com/scionproto/scion/builds/4763
Reviewable status: 3 of 12 files reviewed, all discussions resolved (waiting on @jiceatscion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But go.mod gets still reverted back to go 1.22.7. Is this intentional?
Reviewable status: 3 of 12 files reviewed, all discussions resolved (waiting on @jiceatscion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 9 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)
go.mod
line 131 at r3 (raw file):
) go 1.22.7
I agree with Marc... revert this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is yes. This ensures that downstream dependents of scionproto/scion also do not use the new timer semantics as long as they are using the regular go toolchain.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion)
go.mod
line 131 at r3 (raw file):
Previously, jiceatscion wrote…
I agree with Marc... revert this?
I don't think we should change this until the Go has patched the problem. Note that this just indicates the semantics, not the go version that is being used. See https://go.dev/doc/modules/gomod-ref#go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more context. I wish that I could tell bazel to use go1.22 semantics when building with 1.23. I have not found a way to do so.
Using go 1.22.7 in the go.mod file is actually the proper way to handle such things, the xhack is just there for bazel.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, 1 of 2 files at r2, 9 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)
go.mod
line 131 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
I don't think we should change this until the Go has patched the problem. Note that this just indicates the semantics, not the go version that is being used. See https://go.dev/doc/modules/gomod-ref#go
Ah, ok didn't know how that works. But then, why bother with the xhack. Would it not be simpler and clearer to force bazel to actually build with go 1.22 by setting the version in WORKSPACE? Whatever works though, it's just until the next go release. But still I'm curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 10 of 12 files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @marcfrei)
go.mod
line 131 at r3 (raw file):
Previously, jiceatscion wrote…
Ah, ok didn't know how that works. But then, why bother with the xhack. Would it not be simpler and clearer to force bazel to actually build with go 1.22 by setting the version in WORKSPACE? Whatever works though, it's just until the next go release. But still I'm curious.
Hm. Let me test if that works
b5bc0ab
to
ca91e1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reached out to the bazel community. There is a draft PR that will add support very soon to rules_go bazel-contrib/rules_go#4106
Reviewable status: 10 of 18 files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @marcfrei)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to know. How does that affect the choice of using xhack vs. setting the go version in WORKSPACE, though. Have you decided?
Reviewed 1 of 2 files at r4, 2 of 6 files at r5, all commit messages.
Reviewable status: 13 of 18 files reviewed, 1 unresolved discussion (waiting on @marcfrei and @oncilla)
go.mod
line 131 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Hm. Let me test if that works
The WORKSPACE approach seems to be working (1/20 failure rate is pretty much the normal flakyness of the e2e tests).
f497d89
to
69208e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Reviewable status: 6 of 18 files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @marcfrei)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 6 files at r5, 8 of 8 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, 1 of 2 files at r2, 1 of 2 files at r4, 6 of 6 files at r5, 8 of 8 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)
Until golang/go#69312 is resolved, force the old timer behavior by specifying an older go version in the go.mod file.
Fixes #4606