Updates to build sys and CONTRIBUTING.md #1638
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the build and development process by making the Justfile the main entry point. The changes improve the organization of build and test commands, update the developer documentation accordingly, and fix an issue with SELinux during tests. My review has identified a missing build dependency (jq) which could cause build failures, and a minor typo in the updated CONTRIBUTING.md. Overall, these are great improvements to the project's developer experience.
cgwalters
changed the title
cgwalters
force-pushed
the
build-sys-even-more
branch
from
0acd358 to
f9e73c4
Compare
- Use &Path and not &PathBuf per general style - Include info about which xattr failed - Consistently use with_context + format! Signed-off-by: Colin Walters <[email protected]>
cgwalters
force-pushed
the
build-sys-even-more
branch
from
f9e73c4 to
17caa5c
Compare
There was a problem hiding this comment.
All we need now is a bootc image with all dependencies preinstalled for devs! 😆
This is much nicer though. Making it easier to run tests locally through a single interface is a huge improvement.
Reminder: just --choose exists and is awesome - especially after documenting each entrypoint :)
| - name: Run container tests | ||
| run: | ||
| sudo just test-container |
There was a problem hiding this comment.
Perhaps it may be nicer to invoke sudo within the justfile?
I know it goes against best practice, though if someone must remember which commands require sudo and which ones don't, I know I'd probably just default to sudoing more than needed.
There was a problem hiding this comment.
Ah, so this test-container entrypoint is expected to be ran both rootful and rootless depending on the executing workflow.
Yeah, there's not really anything that can be done here.
I no longer seem to be able to resolve my own threads, so feel free to resolve/ignore this one.
There was a problem hiding this comment.
Thanks for looking at this btw!
Perhaps it may be nicer to invoke sudo within the justfile?
With https://github.com/bootc-dev/bcvk and bcvk to-disk in particular, everything fully supports rootless (and rootful).
There was a problem hiding this comment.
(But I still need to wire up bcvk into the CI on this repo and drop all the sudo)
cgwalters
force-pushed
the
build-sys-even-more
branch
3 times, most recently
from
fcd4a4e to
1768565
Compare
The emphasis here is on trying to have the `Justfile` be the default entrypoint, wrapping other tools. - Replace mentions of podman-bootc with bcvk since I hope the latter supercedes the former - Unify the unit test entrypoint - Set up /var/tmp as a tmpdir to fix the etc merge test (otherwise, selinux failures w/tmp) - Run the unit+container tests in integration.yml - Have `just validate` run in a container Signed-off-by: Colin Walters <[email protected]>
cgwalters
force-pushed
the
build-sys-even-more
branch
from
1768565 to
5f5c728
Compare
jeckersb
left a comment
jeckersb
left a comment
There was a problem hiding this comment.
I absolutely ❤️ this. I think this is the first time I've been able to run our tmt stuff locally with any success.
Things I tested:
just validatejust test-containerjust test-tmt-one test-20-local-upgrade
This is running against a local build of bcvk from git master, running from within a toolbox pointed at the host podman instance via systemd user socket. Worked flawlessly 🥳
| # We intentionally don't gate on this for local builds in cargo.toml | ||
| # because it impedes iteration speed. | ||
| CLIPPY_CONFIG = -A clippy::all -D clippy::correctness -D clippy::suspicious -D clippy::disallowed-methods -Dunused_imports -Ddead_code | ||
| validate-rust: |
There was a problem hiding this comment.
I am going to type make validate-rust probably 50 times before this sinks in 😆
There was a problem hiding this comment.
Yeah fair point, though it's probably good since I think ideally we all switch to just validate.
Though, a big tension for me now is using a toolbox and not devcontainer, one ends up with needing e.g. rust-analyzer on the host system, which means you're building all the Rust code there anyways...
3 participants
The emphasis here is on trying to have
the
Justfilebe the default entrypoint,wrapping other tools.
since I hope the latter supercedes the former
test (otherwise, selinux failures w/tmp)
just validaterun in a containerSigned-off-by: Colin Walters [email protected]
Closes: #1635