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

Add an additional MB of space to the generated FAT partition #397

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

kennystrawnmusic
Copy link
Contributor

@kennystrawnmusic kennystrawnmusic commented Oct 22, 2023

Increasing the image size by an additional megabyte makes the image compile again

Fixes #396

src/fat.rs Outdated
let fat_size_padded_and_rounded = ((needed_size + 1024 * 64 - 1) / MB + 1) * MB;
let fat_size_padded_and_rounded = ((needed_size + 1024 * 64 - 1) / MB + MB) * MB;
Copy link
Member

Choose a reason for hiding this comment

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

Is that increasing the size by a terabyte? Note that the expression in the parentheses is already multiplied by MB, so now it ends up being (... + MB) * MB, which should be a TB if I'm not mistaken. Replacing the 1 by a 2 is probably a more storage efficient fix 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, doing something else then, hold on…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I decided to revert it back to what it was before and instead simply append + MB to the end of that long calculation, since adding a megabyte is far more reasonable than multiplying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally got this down to one commit but it took git reset --hard <the first of my previous commits> and git pull https://github.com/rust-osdev/bootloader main --allow-unrelated-histories --rebase to do it.

kennystrawnmusic added a commit to kennystrawnmusic/bootloader that referenced this pull request Oct 23, 2023
Add MB instead of multiplying
@Freax13
Copy link
Member

Freax13 commented Oct 23, 2023

Could you remove the first commit?

@Freax13
Copy link
Member

Freax13 commented Oct 23, 2023

A revert commit is still a commit, ideally we'd only have a single commit for this pr. See https://stackoverflow.com/a/42522493 on how to remove commits without creating a revert commits.

kennystrawnmusic added a commit to kennystrawnmusic/bootloader that referenced this pull request Oct 23, 2023
Add MB instead of multiplying
@phil-opp phil-opp changed the title Fix #396 Add an additional MB of space to the generated FAT partition Oct 24, 2023
Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

I updated the PR title to be more descriptive and added the line Fixes #396 to the PR description to automatically close the issue after merging.

One minor request: Right now the title of your commit is "Hard reset per @Freax13's request", which is not really useful. Could you change it to something more descriptive? An easy way to do this is to run the following command:

git commit --amend --message "Add an additional MB of space to the generated FAT partition"

src/fat.rs Outdated
@@ -26,7 +26,7 @@ pub fn create_fat_filesystem(
.truncate(true)
.open(out_fat_path)
.unwrap();
let fat_size_padded_and_rounded = ((needed_size + 1024 * 64 - 1) / MB + 1) * MB;
let fat_size_padded_and_rounded = (((needed_size + 1024 * 64 - 1) / MB + 1) * MB) + MB;
Copy link
Member

Choose a reason for hiding this comment

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

To me it's more readable without the extra parentheses:

Suggested change
let fat_size_padded_and_rounded = (((needed_size + 1024 * 64 - 1) / MB + 1) * MB) + MB;
let fat_size_padded_and_rounded = ((needed_size + 1024 * 64 - 1) / MB + 1) * MB + MB;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to use a hard reset and force-push to incorporate both of these changes without also inadvertently turning 1 commit into 2, but done.

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks!

@phil-opp phil-opp merged commit a1b2eb8 into rust-osdev:main Oct 24, 2023
8 checks passed
@phil-opp
Copy link
Member

By the way, I recommend to use git rebase --interactive (or git rebase -i for short) to squash and reword commits. It's much easier to use than git reset --hard in my opinion. For some basic examples, check out e.g. https://www.sitepoint.com/git-interactive-rebase-guide/ .

@phil-opp phil-opp mentioned this pull request Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image builder sporadically throws ENOSPC on macOS even with 72GB of host space available
3 participants