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

build: force old timer behavior #4620

Merged
merged 8 commits into from
Sep 17, 2024
Merged

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Sep 16, 2024

Until golang/go#69312 is resolved, force the old timer behavior by specifying an older go version in the go.mod file.

Fixes #4606

Until golang/go#69312 is resolved,
force the old timer behavior by specifying an older go version
in the go.mod file.
@jiceatscion
Copy link
Contributor

This change is Reviewable

Copy link
Contributor

@jiceatscion jiceatscion left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

Copy link
Contributor Author

@oncilla oncilla left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

Copy link
Contributor

@jiceatscion jiceatscion left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

Copy link
Contributor

@jiceatscion jiceatscion left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

Copy link
Contributor Author

@oncilla oncilla left a 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)

Copy link
Contributor

@marcfrei marcfrei left a 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)

Copy link
Contributor

@jiceatscion jiceatscion left a 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?

Copy link
Contributor Author

@oncilla oncilla left a 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)

Copy link
Contributor Author

@oncilla oncilla left a 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

Copy link
Contributor Author

@oncilla oncilla left a 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)

Copy link
Contributor

@marcfrei marcfrei left a comment

Choose a reason for hiding this comment

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

:lgtm:

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)

Copy link
Contributor

@jiceatscion jiceatscion left a 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.

Copy link
Contributor Author

@oncilla oncilla left a 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

Copy link
Contributor Author

@oncilla oncilla left a 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)

Copy link
Contributor

@jiceatscion jiceatscion left a 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).

Copy link
Contributor Author

@oncilla oncilla left a 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)

Copy link
Contributor

@jiceatscion jiceatscion left a 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)

Copy link
Contributor

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

Copy link
Contributor

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

@oncilla oncilla enabled auto-merge (squash) September 17, 2024 15:54
@oncilla oncilla enabled auto-merge (squash) September 17, 2024 15:54
Copy link
Contributor Author

@oncilla oncilla left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

@oncilla oncilla merged commit f16cdd6 into scionproto:master Sep 17, 2024
6 checks passed
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.

build: the end-2-end tests have become extremely flaky
3 participants