Skip to content

Implement GPT support #12

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Implement GPT support #12

wants to merge 3 commits into from

Conversation

bnuuydev
Copy link

@bnuuydev bnuuydev commented Jun 3, 2025

A few things need doing before this is good to merge:

  • Write some tests
  • Decide which partition types to define (and add their definitions)
  • Some written documentation could be good, but I've given a syntactic definition of how to use it to start with :)
  • Really the GPT code should call into the MBR code to make the protective MBR. I can have a look in a few days at making this change, but happy for someone else to if they get to it before I do. I forgot I already did this.
  • Partition attributes can't be set (custom attributes pose a problem here) (but this can be forgone honestly; it's pretty niche and the current code is sufficient for bootable disk images, which is the main purpose of this I guess)
  • Only supports GPT with a protective MBR for now, no hybrid MBR. Hybrid MBRs are problematic for a number of reasons, so it might be best just to forget about them for now.

The code might not compile since as I was working on master. I think I've made sure the code is right now, but be sure that they are correct before merging :)

Copy link
Collaborator

@Khitiara Khitiara left a comment

Choose a reason for hiding this comment

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

mostly LGTM (also scooped me, nice), got a few minor nits is all

@Khitiara Khitiara self-assigned this Jun 3, 2025
@Khitiara
Copy link
Collaborator

Khitiara commented Jun 3, 2025

also if you want and run out of time for this I can work on any finishing stuff

@bnuuydev
Copy link
Author

bnuuydev commented Jun 3, 2025

Would appreciate that! Sorry for stepping on your toes a bit with this btw, I'd written it a few weeks ago but had been too busy to make the PR >_<

@Khitiara
Copy link
Collaborator

Khitiara commented Jun 3, 2025

no worries, you got a heck of a lot further than i did

@Khitiara
Copy link
Collaborator

Khitiara commented Jun 3, 2025

addressed the nits and pushed an update to the PR

@Khitiara Khitiara requested a review from ikskuh June 3, 2025 21:23
@bnuuydev
Copy link
Author

bnuuydev commented Jun 3, 2025

Also make sure not to merge this before we have some tests. I'll write some up in a couple of days if you don't get to it first :)

@Khitiara
Copy link
Collaborator

Khitiara commented Jun 4, 2025

I'll also be switching my os over to this pr branch at some point in a few days to try it out in a real scenario

@Khitiara Khitiara mentioned this pull request Jun 5, 2025
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.

2 participants