-
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: prevent go from linking libresolv dynamically. #4394
Conversation
This is default (https://pkg.go.dev/[email protected]#hdr-Name_Resolution). It creates an indirect dependency on a fresh version of glibc. As a result, it prevents our binaries from running on not-so-fesh docker images.
…flag in the config_setting defs.
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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @jiceatscion)
a discussion (no related file):
Is this really required?
I think you can just build against an older glibc version that is supported by your target base image.
E.g., you could use https://github.com/aspect-build/gcc-toolchain which uses glibc 2.26.
Which docker base image are you trying to use?
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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @jiceatscion)
a discussion (no related file):
Previously, oncilla (Dominik Roos) wrote…
Is this really required?
I think you can just build against an older glibc version that is supported by your target base image.
E.g., you could use https://github.com/aspect-build/gcc-toolchain which uses glibc 2.26.Which docker base image are you trying to use?
(I guess the underlying issue is that your locally installed c compiler is using newer glibc version than what is supported)
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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @oncilla)
a discussion (no related file):
Previously, oncilla (Dominik Roos) wrote…
(I guess the underlying issue is that your locally installed c compiler is using newer glibc version than what is supported)
This is no more required than switching to sqlite_modernc was. Same reason. Yes, there are other ways to solve the problem: cross build to target older run-times, or build docker images compatible with the build system. The former would make sense if we intended to build for a specific set of platforms. The latter would be more of the right thing to do if we intend to deliver target-agnostic source-code that one can build on and for one's choice's system.
At the moment, I think that target-agnostic is our focus. We (SCION Association) have no near-term plan to include support packages for specific target platforms. So, adding cross-build support right now wouldn't be a great use of my time (but possibly that's something Anapaya would have a vested interest in).
Making docker images compatible with the build system however isn't immediate either. In the meantime, I, for one, cannot run integration tests anymore. A problem that I can solve for myself and everyone else similarly affected by removing the issue's trigger at basically zero cost to anybody. Any particular reason to want libresolv?
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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @jiceatscion)
a discussion (no related file):
(but possibly that's something Anapaya would have a vested interest in).
Not particularly, we build and distribute the packages in our custom way. But internally, we use the linked aspect project to guarantee hermetic builds and do not rely on the host installed compiler.
At the moment, I think that target-agnostic is our focus.
I remember OSX having issues with the Go only DNS resolver.
(https://danp.net/posts/macos-dns-change-in-go-1-20/)
Maybe this has been resolved in the meantime (but I'm also not an OSX user...)
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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @oncilla)
a discussion (no related file):
Previously, oncilla (Dominik Roos) wrote…
(but possibly that's something Anapaya would have a vested interest in).
Not particularly, we build and distribute the packages in our custom way. But internally, we use the linked aspect project to guarantee hermetic builds and do not rely on the host installed compiler.
At the moment, I think that target-agnostic is our focus.
I remember OSX having issues with the Go only DNS resolver.
(https://danp.net/posts/macos-dns-change-in-go-1-20/)
Maybe this has been resolved in the meantime (but I'm also not an OSX user...)
That'd be a drag if that had an effect on our own users, but I don't see the use case here. Are there people building SCION for MacOS? If that works, then I guess they won't be using the containers, so indeed my change isn't even useful. Unfortunately there doesn't seem to be a way to put conditional expressions in .bazelrc. However, may be someone building on MacOS could override that line in their own ~/.bazelrc?
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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @jiceatscion)
a discussion (no related file):
Previously, jiceatscion wrote…
That'd be a drag if that had an effect on our own users, but I don't see the use case here. Are there people building SCION for MacOS? If that works, then I guess they won't be using the containers, so indeed my change isn't even useful. Unfortunately there doesn't seem to be a way to put conditional expressions in .bazelrc. However, may be someone building on MacOS could override that line in their own ~/.bazelrc?
There have been in the past. I'm not sure if they are still around. I just brought it up because your mentioning being target agnostic, but AFAIU using Go only supports a subset of the targets
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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @oncilla)
a discussion (no related file):
Previously, oncilla (Dominik Roos) wrote…
There have been in the past. I'm not sure if they are still around. I just brought it up because your mentioning being target agnostic, but AFAIU using Go only supports a subset of the targets
Yeah... I wasn't saying super-portable. It's still mostly Linux, I think.
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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @oncilla)
a discussion (no related file):
Previously, jiceatscion wrote…
Yeah... I wasn't saying super-portable. It's still mostly Linux, I think.
Good news. I found how one puts platform specific flags in .bazelrc. Problem solved. No MacOS user harmed by this change.
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 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @oncilla)
a discussion (no related file):
Previously, jiceatscion wrote…
Good news. I found how one puts platform specific flags in .bazelrc. Problem solved. No MacOS user harmed by this change.
This seems ok to me.
For what it's worth, I don't think DNS is critical in any of the SCION binaries. While some configuration files do ostensibly support host names, these names are only resolved at startup. Functionally, this is mostly equivalent to resolving the names manually at the time of configuration.
I'm not sure the special case for OSX is really necessary, but as you found a very simple solution it doesn't seem to hurt keeping it now.
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 @matzf and @oncilla)
a discussion (no related file):
Previously, matzf (Matthias Frei) wrote…
This seems ok to me.
For what it's worth, I don't think DNS is critical in any of the SCION binaries. While some configuration files do ostensibly support host names, these names are only resolved at startup. Functionally, this is mostly equivalent to resolving the names manually at the time of configuration.
I'm not sure the special case for OSX is really necessary, but as you found a very simple solution it doesn't seem to hurt keeping it now.
Thanks! Dominik?
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 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @matzf)
a discussion (no related file):
Previously, jiceatscion wrote…
Thanks! Dominik?
It's not the route I would go, but I also don't have strong opinions.
Resolved and
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 @jiceatscion)
I had the disctionary backwards :-)
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 r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)
Danger danger: I'm merging despite failing buildkite run; the failing is only due to the updated buildkite runner stack, which will require some small fixes in our pipeline to work (see #4397). As this change here is also required to make the tests run on the new stack, I'm merging this now. |
This is default (https://pkg.go.dev/[email protected]#hdr-Name_Resolution). It creates an indirect dependency on a fresh version of glibc. As a result, it prevents our binaries from running on not-so-fesh docker base images.
This change is