Skip to content

install: Two rootfs cleanups #909

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
Nov 20, 2024
Merged

Conversation

cgwalters
Copy link
Collaborator

install: Reduce usage of absolute path for rootfs

In the install flow we have both rootfs and rootfs_fd which
hold the physical root. Using fd-relative accesses where we
can provides a lot of advantages, so switch most uses over
to the file descriptor.

Signed-off-by: Colin Walters walters@verbum.org


install: Rename rootfs -> physical_root

In the install flow we juggle three file systems in general:

  • The container/host root
  • The physical root
  • The deployment root

"rootfs" in theory could be any of those three. In the install code
it's the physical (target) root, so rename the variable
to clarify.

Signed-off-by: Colin Walters walters@verbum.org


In the install flow we have both `rootfs` and `rootfs_fd` which
hold the physical root. Using fd-relative accesses where we
can provides a lot of advantages, so switch most uses over
to the file descriptor.

Signed-off-by: Colin Walters <walters@verbum.org>
In the install flow we juggle *three* file systems in general:

- The container/host root
- The physical root
- The deployment root

"rootfs" in theory could be any of those three. In the install code
it's the physical (target) root, so rename the variable
to clarify.

Signed-off-by: Colin Walters <walters@verbum.org>
@github-actions github-actions bot added the area/install Issues related to `bootc install` label Nov 19, 2024
@@ -661,7 +661,6 @@ async fn initialize_ostree_root(state: &State, root_setup: &RootSetup) -> Result
)?;
}

let sysroot = ostree::Sysroot::new(Some(&gio::File::for_path(rootfs)));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note here we somehow ended up with two sysroot objects pointlessly, so this drops the second.

@cgwalters
Copy link
Collaborator Author

cgwalters commented Nov 19, 2024

                    
                      × Assignment operations require a variable.
                        ╭─[/var/tmp/tmt/run-002/plans/test-20-local-upgrade/tree/tests/booted/test-image-pushpull-upgrade.nu:86:5]
                     85 │     let booted_digest = $booted.imageDigest
                     86 │     print booted_digest = $booted_digest
                        ·     ─────────┬─────────
                        ·              ╰── needs to be a variable
                     87 │     # We should already be fetching updates from container storage
                        ╰────
                      help: try assigning to a variable or a cell path of a variable
                    

Hmm, I think the nushell update here in EPEL (from yesterday) broke our test scripts. Looking at this

@cgwalters
Copy link
Collaborator Author

nushell/nushell#14333 is also related to this I think

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.

lgtm

@cgwalters cgwalters merged commit 0f6496c into bootc-dev:main Nov 20, 2024
29 of 32 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`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants