-
Notifications
You must be signed in to change notification settings - Fork 1.2k
build: add ability to interactively run guix builds, use container that has unprivileged user #5449
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
Conversation
|
Makes me sad this had to get moved over to ubuntu, instead of alpine, but probably fine. Started my build, let's make sure it works, seems good so far |
|
For some reason my arm dash-qt binary doesn't match... everything else matches |
|
arm-linux-gnueabihf dash-qt is non-deterministic... CI had I have (twice) |
|
I got 2 binaries: There's difference in disassembled code of function @kittywhiskers need to backport this one: https://github.com/bitcoin/bitcoin/pull/25643/files note for myself (and for further cases): need to install |
|
We can probably move forward with this PR then and backport that in the next set of guix backports |
PastaPastaPasta
left a comment
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.
utACK for squash merge
UdjinM6
left a comment
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.
utACK
|
I still have couple extra comments that I accidentally put to wrong PR.
|
|
Good thoughts, but I'm gonna merge this so we can move forward with guix part 3; feel free to do a PR with those changes |
|
The reason why we cannot de-duplicate is because both We could de-duplicate it but that would require the scripts to be modified to be relative, which defeats the point of embedding them into PATH since they won't "run-anywhere", which makes the interactive prompt harder to use. As for binding |
…#5464) ## Additional Information * Based on suggestions by @knst made [here](#5449 (comment)) and [here](#5426 (comment))
…in#26257, bump to Python 3.10, mypy 0.981, fix Docker group assignment, minor housekeeping (misc. changes: part 2) 82723dc fix: don't forget to assign user to group if group exists (Kittywhiskers Van Gogh) 066d409 chore: remove outdated `boot2docker` comment (Kittywhiskers Van Gogh) 29e98e3 chore: document `USER_ID` and `GROUP_ID` in `docker-compose.yml` (Kittywhiskers Van Gogh) 6ea897a chore: drop unmaintained Guix container (Kittywhiskers Van Gogh) 6a1786c fix: resolve `test: =: unary operator expected` error (Kittywhiskers Van Gogh) d6489f0 fix: make copy of `skip` in `GetStackFrames` to avoid clobbering (Kittywhiskers Van Gogh) 4110ff3 lint: mypy 0.981 (Kittywhiskers Van Gogh) 80a44e9 partial bitcoin#26257: python linter flake8 E275 fixup, update dependencies (Kittywhiskers Van Gogh) 7b80dfb merge bitcoin#28347: replace deprecated pkg_resources with importlib.metadata (Kittywhiskers Van Gogh) 04ac20a partial bitcoin#30527: Bump python minimum supported version to 3.10 (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6988 * Python 3.9's security support elapsed on 31st Oct '25 ([source](https://endoflife.date/python)), in response to that we are switching over to Python 3.10. * Due to [python/mypy#13627](python/mypy#13627) arising as a result of this bump, we also had to upgrade `mypy` to 0.981. Note that this is a divergence from upstream as they opted to upgrade to `mypy` 1.x (see [bitcoin#28009](bitcoin#28009)) but the changes needed to do so are too disruptive given this PR's larger context. Future backports should feel free to overwrite the mypy version and realign with upstream. * CI identified potential clobbering ([build](https://github.com/kwvg/dash/actions/runs/19434684086/job/55606106842#step:8:1804)) in Windows stacktraces code that started to build after [dash#6966](#6966) (see below). Additionally, C++20 has deprecated certain operators like {in,de}crementation courtesy of [P1152](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1152r3.html), which required additional adaptation. <details> <summary>Build error:</summary> ``` stacktraces.cpp: In function ‘std::vector<long long unsigned int> GetStackFrames(size_t, size_t, const CONTEXT*)’: stacktraces.cpp:167:56: error: variable ‘skip’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered] 167 | static __attribute__((noinline)) std::vector<uint64_t> GetStackFrames(size_t skip, size_t max_frames, const CONTEXT* pContext = nullptr) | ^~~~~~~~~~~~~~ cc1plus: all warnings being treated as errors ``` </details> * The `AM_CONDITIONAL` for `CRASH_HOOKS_WRAPPED_CXX_ABI` wasn't evaluated due to missing quotations (see below), this has been resolved. <details> <summary>Configure:</summary> ``` checking whether the linker accepts -Wl,-export-dynamic... no checking whether the linker accepts -rdynamic... yes checking whether C++ compiler accepts -gdwarf-4... yes checking whether C++ compiler accepts -fno-standalone-debug... yes checking whether the linker accepts -Wl,-wrap=__cxa_allocate_exception... no ./configure: line 30603: test: =: unary operator expected checking for Linux getrandom syscall... no checking for getentropy via random.h... yes checking for sysctl... yes checking for sysctl KERN_ARND... no checking for fdatasync... no ``` </details> * Currently we offer two containers for Guix builds meant for developers who aren't on Linux, one was introduced by [dash#5285](#5285) (based on [fanquake/core-review](https://github.com/fanquake/core-review)'s container, [source](https://github.com/fanquake/core-review/blob/d8cf188214879ea1b095e2ba34ca5c23dbc3ebd2/guix/imagefile)) and the other introduced by [dash#5449](#5449), the former does not seem to get much use and has been out of sync with its upstream source for a while. As the second container is used by some developers and is updated and maintained to fit Dash's specific needs, the first container has been dropped and documentation has been updated to reflect the same. * As Docker has matured with the WSL2 backend on Windows and the Apple Virtualization framework on macOS, boot2docker has been deprecated (see [boot2docker/boot2docker#1408](boot2docker/boot2docker#1408)) and the comment referencing it has been dropped. * To avoid permissions issues with mounting directories, containers come with `USER_ID` and `GROUP_ID` args ([source](https://github.com/dashpay/dash/blob/23de96916a8b8f97a2c408bb76da1d2149d7227c/contrib/containers/ci/ci-slim.Dockerfile#L123-L131)) that need to be specified at build time if the mount needs different permissions (as is often the case on macOS). To make that option more explicitly clear, it has been specified in `docker-compose.yml` with default values filled in. * [dash#6929](#6929) introduced a fix to prevent unexpected container build failure due to conflicting groups (most commonly GID 20 that on macOS is `staff` but on Linux is `dialout`) but the fix did _not_ assign the default user to that group, that has been resolved here. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 82723dc ~with one nit~ Tree-SHA512: 0e906d1a7fea9edc52a40a6a6971fa6b2599674e97e99c65a220f99cc44be78f4290be8fb9af7782cac416bcdd2338b7f17a5c50b5fdcf727b1cf84fe44c8686
Additional Notes
/gnu/storeand/var/guixare pre-populated when extracting the Guix archive, which is done during the image building process./gnu/storeas read-only for anything that isn't the daemonguix-start) and will need to be updated if the SDK version is updatedThe container runs as privileged process and all operations are being done as the root user (no unprivileged user exists within the container due to its barebones nature)No longer true, as we're not inheriting the Alpine container. An unprivileged user is used for git-specific operations with a configurable UID/GID pair which should be matched with the host user.https://bordeaux.guix.gnu.organdhttps://ci.guix.gnu.org(seeentrypoint) as it fetches substitutions from it to avoid re-building unpatched/unmodified packages by fetching them from a remote server.Motivation
@PastaPastaPasta was interested in the possibility of being able to run Guix builds as it is run by CI interactively with the fewest commands possible. Since my personal devenv is entirely defined using Docker, I had some experience with interactive containers.
We initially tried using the Alpine container (and earlier versions of this PR built upon this container) already available in
developbut two problems arose:build-push-action@v2is not particularly cooperative withedrevo/dockerfile-plus(which is how we get theINCLUDE+macro).dockerfile-plusdoubly so. Your context directory is loaded into the builder and therefore, it is imperative to keep the context as minimal as possible, at the risk of increased resource consumption and longer build times.build-push-actionwould not cooperate despite efforts accepting the tradeoff and passing the repository root as the contextdockerfile-plusThe context problem still persisted after moving to the Ubuntu-based container (which matches the rest of our CI) but Docker implements something called "additional contexts" that allow you to do things like
COPYoperations with a different directory as the base.