Skip to content

install: Add a generic install finalize #1157

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

Merged
merged 2 commits into from
Mar 1, 2025

Conversation

cgwalters
Copy link
Collaborator

@cgwalters cgwalters commented Feb 28, 2025

Basically I want to get Anaconda to run this, then we can perform arbitrary fixups on whatever it did between the install and reboot without changing Anaconda's code.

Motivated specifically by #971

This also applies to user %post scripts for example; maybe those break the bootloader entries in /boot; we have the opportunity to catch such things here.

Or we may choose to start forcibly relabeling the target /etc.

@github-actions github-actions bot added documentation Improvements or additions to documentation area/install Issues related to `bootc install` labels Feb 28, 2025
@cgwalters cgwalters force-pushed the install-finalize branch 3 times, most recently from 9ed3e11 to 44c632e Compare February 28, 2025 22:40
Copy link
Collaborator

@jeckersb jeckersb left a comment

Choose a reason for hiding this comment

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

Just lint nits, but we've accumulated a fair number of those recently and we can do all of that cleanup in a followup

This took me some thinking and experimenting. Basically we want:

- Hard deny some warnings (this is covered by the Cargo.toml
  workspace.lints.rust)
- Gate merging to main in CI on an exact set of warnings we
  want to forbid, but *without* also using a blanket
  -Dwarning deny policy because that could break our build
  when the compiler revs.
- A corollary to the previous: allow developing locally
  without killing the build just because
  you have an unused import or some dead code (for example).
  So we don't want to add `dead_code = deny` into the Cargo.toml.
- Be able to easily reproduce locally what CI is gating on
  in an efficient way.

We already had `make validate-rust` which was intending to navigate
this, but what was missing was the "deny extended set of warnings"
so we got code committed to git main which hit `unused_imports`.

Clippy upstream docs recommend the `RUSTFLAGS = -Dwarnings`
approach in e.g.
https://doc.rust-lang.org/clippy/continuous_integration/github_actions.html
but again I think this is a problem because it can break with
updated Rust/clippy versions (unless you pin on those, but that
becomes a pain in and of itself).

The problem also with doing `RUSTFLAGS = -Dwarnings` *locally*
is it blows out the cargo cache.

So here's the solution I came to: We run `cargo clippy -A clippy:all`,
and then deny some specific clippy lints *and* the core Rust
warnings we want (`unused_imports` type things) at this stage.
The advantage is this doesn't blow out the main Cargo cache,
and I can easily reproduce locally exactly what CI would gate on.

Also while we're here, add `make fix-rust` which is a handy
way to use the existing `clippy --fix` to locally fix things
like unused imports as well as other machine-applicable
things that are in e.g. `clippy::suspicious`.

Signed-off-by: Colin Walters <[email protected]>
@cgwalters
Copy link
Collaborator Author

Just lint nits, but we've accumulated a fair number of those recently and we can do all of that cleanup in a followup

OK yeah, we should definitely be gating merges to git main on at least things like unused_imports (and probably most things that rustc would warn about by default, but I don't like a blanket "deny all warnings in CI" policy either)

I ended up with #1159

Basically I want to get Anaconda to run this, then we
can perform arbitrary fixups on whatever it did
between the install and reboot without changing Anaconda's
code.

This also applies to user `%post` scripts for example;
maybe those break the bootloader entries in /boot;
we have the opportunity to catch such things here.

Or we may choose to start forcibly relabeling the target
`/etc`.

Signed-off-by: Colin Walters <[email protected]>
@cgwalters cgwalters enabled auto-merge March 1, 2025 18:45
@cgwalters cgwalters merged commit eb585e0 into bootc-dev:main Mar 1, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/install Issues related to `bootc install` documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants