-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
ci: containerize ubuntu cli jobs #9708
base: master
Are you sure you want to change the base?
Conversation
d0c51e0
to
7eb6f55
Compare
Running 20.04 in a container makes sense. Regarding 22.04, how much slower does this make the build? It seems you could just move it when needed, the change would not be terribly complex, and will likely coincide with the promotion of 24 to "popular" and 20 to "no longer supported". |
Initializing the container and installing packages that would have been installed by default outside of the container takes at most 10 seconds. This is insignificant compared to the usual variance between runs, which is on the order of several minutes. |
Containerization also has the added benefit of accurately representing the target development environment, so we don't run into a repeat of #9128. Removing the bundled packages can take as much as 10 minutes. Edit: Using |
7eb6f55
to
72d5da8
Compare
You can easily, and quickly, drop the contents of
|
- name: build | ||
run: | | ||
${{env.CCACHE_SETTINGS}} | ||
${{env.BUILD_DEFAULT_LINUX}} | ||
libwallet-ubuntu: | ||
name: "Ubuntu 20.04 (libwallet)" |
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.
Why does this action not also run in the container?
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 scheduled for removal in #9706, I forgot to mention the soft dependency. Leaving it like this avoids a conflict.
run: | | ||
${{env.CCACHE_SETTINGS}} | ||
${{env.BUILD_DEFAULT_LINUX}} | ||
cmake --build build --target test | ||
# ARCH="default" (not "native") ensures, that a different execution host can execute binaries compiled elsewhere. |
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.
Not related to this PR, but it seems this comment very much lost its anchor at some point (I believe it was intended to apply to BUILD_DEFAULT_LINUX; the part about BUILD_SHARED_LIBS no longer appears anywhere in the file). Do you want to open a separate PR at some point to correct the drift?
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, I made a note of this.
Some clarification of my feedback thusfar may be in order; I meant to signify approval of this line:
As far as performance, the relevant measure would be the total addition to build time, not just the setup phase. Because I consider faithful representation of the target environment most important, and "just drop /usr/include" seems a bit finicky, I would favor the container approach even if it adds, say, a minute of overhead; past that, and the tradeoff becomes less clear in my view. |
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.
The changes here seem reasonable. The test failures owe to a flawed test methodology (fixed in #9709, though I may post additional improvements to that test suite after yours have been merged).
GitHub will begin deprecation of the Ubuntu 20.04 runner in Feb 2025. We support Ubuntu 20.04 until at least Apr 2026.
Proactively containerize both Ubuntu 20.04 and 22.04 to avoid future disruptions.
Edit: drafted because of failing tests: