Skip to content
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

Cure APFS/Fstabs on Mac #246

Merged
merged 20 commits into from Mar 8, 2023
Merged

Conversation

Hoverbear
Copy link
Contributor

@Hoverbear Hoverbear commented Feb 13, 2023

Description

Create the CreateFstabEntry action how to accept a planned CreateApfsVolume and, using that, decide if it should skip updating the fstab.

Teaches CreateFstabEntry to recognize and edit existing /nix based fstab entries to correct them.

Checklist
  • Formatted with cargo fmt
  • Built with nix build
  • Ran flake checks with nix flake check
  • Added or updated relevant tests (leave unchecked if not applicable)
  • Added or updated relevant documentation (leave unchecked if not applicable)
  • Linked to related issues (leave unchecked if not applicable)
Validating with install.determinate.systems

If a maintainer has added the upload to s3 label to this PR, it will become available for installation via install.determinate.systems:

curl --proto '=https' --tlsv1.2 -sSf -L https://install.determinate.systems/nix/pr/$PR_NUMBER | sh -s -- install

@Hoverbear Hoverbear self-assigned this Feb 13, 2023
@Hoverbear
Copy link
Contributor Author

I need to do some thorough testing on this.

@Hoverbear
Copy link
Contributor Author

Doing this has made me want a tracing_revert_synopsis as well...

@Hoverbear
Copy link
Contributor Author

We now report that we are about to update an entry:

image

However, the volume handling needs to improve:
image

@Hoverbear Hoverbear added this to the v0.3.1 milestone Feb 15, 2023
@Hoverbear Hoverbear removed this from the v0.3.1 milestone Feb 22, 2023
@Hoverbear
Copy link
Contributor Author

I'll rebase this on top #296 once it merges and it should be r4r

@Hoverbear Hoverbear added the upload to s3 The labeled PR is allowed to upload its artifacts to S3 for easy testing label Mar 6, 2023
@Hoverbear Hoverbear marked this pull request as ready for review March 6, 2023 18:44
@cole-h cole-h self-requested a review March 7, 2023 20:01
@Hoverbear Hoverbear requested a review from cole-h March 8, 2023 18:32
@@ -32,7 +32,7 @@ impl BootstrapLaunchctlService {
let mut command = Command::new("launchctl");
command.process_group(0);
command.arg("print");
command.arg(format!("{domain}/{service}"));
command.arg("{domain}/{service}");
Copy link
Member

Choose a reason for hiding this comment

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

.....this seems wrong. Am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YOu're right, oh my goodness. Wait, how did the tests pass?

Copy link
Member

Choose a reason for hiding this comment

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

That's a great question.

We should probably make a Command wrapper that will automatically trace the commands being run locally, so we could see if this appeared in the logs...

But I think that's because the command spawned properly (so the .map_err into ActionError::command(...) never happened), but that it was just an error -- and in that case all we do is mark it uncompleted. The lack of a format! would have just made this unconditionally false (and thus always uncompleted).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have this -- however it also gobbles some errors we don't necessarily want it to (such as in this case):

nix-installer/src/lib.rs

Lines 94 to 104 in f9ab680

async fn execute_command(command: &mut Command) -> Result<Output, ActionError> {
tracing::trace!("Executing `{:?}`", command.as_std());
let output = command
.output()
.await
.map_err(|e| ActionError::command(command, e))?;
match output.status.success() {
true => Ok(output),
false => Err(ActionError::command_output(command, output)),
}
}

cole-h

This comment was marked as outdated.

@Hoverbear
Copy link
Contributor Author

@cole-h If you could help me know which 0o100644 you are referring to, I'd love to fix it! I don't see it anywhere in the branch.

harmonic on  hoverbear/ds-676-curable-fstabapfs-on-mac [$] is 📦 v0.4.0 via 🦀 v1.67.1 via ❄️  impure (nix-install-shell-env) 
❯ rg "0o100644"

harmonic on  hoverbear/ds-676-curable-fstabapfs-on-mac [$] is 📦 v0.4.0 via 🦀 v1.67.1 via ❄️  impure (nix-install-shell-env) 

@cole-h
Copy link
Member

cole-h commented Mar 8, 2023

..........embarrassing. I was looking at an old commit.

You saw nothing.

@Hoverbear Hoverbear merged commit 07a48fe into main Mar 8, 2023
@Hoverbear Hoverbear deleted the hoverbear/ds-676-curable-fstabapfs-on-mac branch March 8, 2023 20:49
@Hoverbear Hoverbear added this to the v0.6.0 milestone Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upload to s3 The labeled PR is allowed to upload its artifacts to S3 for easy testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants