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 partition labels support #30

Closed
wants to merge 1 commit into from

Conversation

Baitinq
Copy link
Contributor

@Baitinq Baitinq commented Aug 26, 2022

No description provided.

@Baitinq
Copy link
Contributor Author

Baitinq commented Aug 26, 2022

Fixes #29

@Lassulus
Copy link
Collaborator

huh does is need to be positional or can we parametrize the argument?

@@ -142,7 +142,7 @@ let
env = helper.device-id q.device;
in
''
parted -s "''${${env}}" mkpart ${x.part-type} ${x.fs-type or ""} ${x.start} ${x.end}
parted -s "''${${env}}" mkpart ${x.part-type} ${x.part-label} ${x.fs-type or ""} ${x.start} ${x.end}
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is correct?

  mkpart PART-TYPE [FS-TYPE] START END     make a partition

I don't see any label support in parted for this.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@Baitinq Baitinq Aug 26, 2022

Choose a reason for hiding this comment

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

@Mic92 Im seeing conflicting info. On the arch wiki for parted (https://wiki.archlinux.org/title/Parted) it says:

(parted) mkpart part-type-or-part-label fs-type start end

    part-type-or-part-label is interpreted differently based on the partition table:
        MBR: the parameter is interpreted as part-type, which can be one of primary, extended or logical.
        GPT: the parameter is interpreted as part-label, which sets the [PARTLABEL](https://wiki.archlinux.org/title/PARTLABEL) attribute of the partition. The partition label always has to be set, since mkpart does not allow to create partitions with empty label.

EDIT: With this I understand we can only set labels using mkpart if using GPT (by using the method in this pr but not setting any fs-type, effectively replacing fs-type by fs-label)

Copy link
Member

Choose a reason for hiding this comment

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

Ok. In this case I would pass down the format type in create.table down to create.partition:

    ${concatStrings (imap (index: create-f (q // { inherit index format; })) x.partitions)}

And than only pass either x.part-type or x.part-label to parted based on the format type (gpt or mbr).
Otherwise disko might pass too many arguments to mkpart

Copy link
Contributor

@yajo yajo Aug 29, 2022

Choose a reason for hiding this comment

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

The man page is interesting:

          mkpart [part-type name fs-type] start end
                 Create a new partition. part-type may be specified
                 only with msdos and dvh partition tables, it should
                 be one of "primary", "logical", or "extended".
                 name is required for GPT partition tables and fs-
                 type is optional.  fs-type can be one of "btrfs",
                 "ext2", "ext3", "ext4", "fat16", "fat32", "hfs",
                 "hfs+", "linux-swap", "ntfs", "reiserfs", "udf", or
                 "xfs".

          name partition name
                 Set the name of partition to name. This option
                 works only on Mac, PC98, and GPT disklabels. The
                 name can be placed in double quotes, if necessary.
                 And depending on the shell may need to also be
                 wrapped in single quotes so that the shell doesn't
                 strip off the double quotes.

It seems to indicate the same as https://github.com/nix-community/disko/pull/30/files#r956295248.

Maybe a good option is to assert that you're getting either a part-type for msdos and dvh tables, or a part-label for gpt tables. Or to use parted name to be specific about what it's doing.

@yajo yajo mentioned this pull request Aug 29, 2022
@Lassulus
Copy link
Collaborator

Lassulus commented Oct 2, 2022

this should now be possible already with the version on master

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.

None yet

4 participants