Some more lanzatool refactoring #43

Merged
nikstur merged 3 commits from some-more-lanzatool-refactoring into master 2023-01-01 23:41:13 +00:00
nikstur commented 2022-12-31 01:59:29 +00:00 (Migrated from github.com)

Implements some requested changes from #34.

Implements some requested changes from #34.
RaitoBezarius commented 2022-12-31 20:52:45 +00:00 (Migrated from github.com)

@nikstur Do you think we could get the "alternative Nix store" path feature and make it a parameter or something smart?

@nikstur Do you think we could get the "alternative Nix store" path feature and make it a parameter or something smart?
RaitoBezarius (Migrated from github.com) reviewed 2022-12-31 20:54:09 +00:00
RaitoBezarius (Migrated from github.com) commented 2022-12-31 20:54:07 +00:00

IMHO, a UEFI String is a UTF-16 string, but String is a UTF-8 String AFAIK.

IMHO, a UEFI String is a UTF-16 string, but String is a UTF-8 String AFAIK.
RaitoBezarius (Migrated from github.com) reviewed 2022-12-31 20:54:33 +00:00
RaitoBezarius (Migrated from github.com) commented 2022-12-31 20:54:33 +00:00

This function is more a "UNIX paths to Windows-style paths" AFAIK

This function is more a "UNIX paths to Windows-style paths" AFAIK
RaitoBezarius (Migrated from github.com) reviewed 2022-12-31 20:58:00 +00:00
RaitoBezarius (Migrated from github.com) commented 2022-12-31 20:57:59 +00:00

I wonder if windows::Path could be directly imported.

I wonder if `windows::Path` could be directly imported.
nikstur commented 2023-01-01 15:53:10 +00:00 (Migrated from github.com)

@nikstur Do you think we could get the "alternative Nix store" path feature and make it a parameter or something smart?

From some research, I got the impression, that an alternative Nix Store path is not really a well-supported feature of Nix e.g. https://github.com/NixOS/nix/issues/1971. I'd say we just do not explicitly support an alternative path. With the implementation from #42 lanzatool doesn't really know about /nix/store at all anyways, so other store locations should just work. I implemented it that way so I can test with files in a temporary directory instead of /nix/store. (See: https://github.com/nix-community/lanzaboote/blob/master/rust/lanzatool/src/esp.rs#L56)

> @nikstur Do you think we could get the "alternative Nix store" path feature and make it a parameter or something smart? From some research, I got the impression, that an alternative Nix Store path is not _really_ a well-supported feature of Nix e.g. https://github.com/NixOS/nix/issues/1971. I'd say we just do not explicitly support an alternative path. With the implementation from #42 lanzatool doesn't really know about /nix/store at all anyways, so other store locations should just work. I implemented it that way so I can test with files in a temporary directory instead of /nix/store. (See: https://github.com/nix-community/lanzaboote/blob/master/rust/lanzatool/src/esp.rs#L56)
nikstur (Migrated from github.com) reviewed 2023-01-01 16:06:20 +00:00
nikstur (Migrated from github.com) commented 2023-01-01 16:06:20 +00:00

From what I can tell you can't easily convert a path using the stdlib. I found this library: https://github.com/r-efi/r-efi-string.
From the readme it seems that UEFI strings are even more complicated than just UTF-16. Do you think the comment or function name is not fitting? I'm not sure we gain much if we encode the complexities of UEFI Strings into e.g. the function name.

From what I can tell you can't easily convert a path using the stdlib. I found this library: https://github.com/r-efi/r-efi-string. From the readme it seems that UEFI strings are even more complicated than just UTF-16. Do you think the comment or function name is not fitting? I'm not sure we gain much if we encode the complexities of UEFI Strings into e.g. the function name.
RaitoBezarius commented 2023-01-01 19:26:22 +00:00 (Migrated from github.com)

@nikstur Do you think we could get the "alternative Nix store" path feature and make it a parameter or something smart?

From some research, I got the impression, that an alternative Nix Store path is not really a well-supported feature of Nix e.g. NixOS/nix#1971.

It definitely is, the issue you linked is about rewriting the paths which is something different and could be useful to reuse the NixOS cache in an alternative store path.
But, if you do nix-build x --store ~/nix, it will create a new Nix store at this path.

Some people do run Nix store paths at alternative locations, certainly, this is very edgy, but I'm not sure if it cost that much to support it as a flag.

I'd say we just do not explicitly support an alternative path. With the implementation from #42 lanzatool doesn't really know about /nix/store at all anyways, so other store locations should just work.

Hm, that's fair. We can wait for a feature request then.

I implemented it that way so I can test with files in a temporary directory instead of /nix/store. (See: https://github.com/nix-community/lanzaboote/blob/master/rust/lanzatool/src/esp.rs#L56)

Great!

> > @nikstur Do you think we could get the "alternative Nix store" path feature and make it a parameter or something smart? > > From some research, I got the impression, that an alternative Nix Store path is not _really_ a well-supported feature of Nix e.g. [NixOS/nix#1971](https://github.com/NixOS/nix/issues/1971). It definitely is, the issue you linked is about rewriting the paths which is something different and could be useful to reuse the NixOS cache in an alternative store path. But, if you do `nix-build x --store ~/nix`, it will create a new Nix store at this path. Some people do run Nix store paths at alternative locations, certainly, this is very edgy, but I'm not sure if it cost that much to support it as a flag. > I'd say we just do not explicitly support an alternative path. With the implementation from #42 lanzatool doesn't really know about /nix/store at all anyways, so other store locations should just work. Hm, that's fair. We can wait for a feature request then. > I implemented it that way so I can test with files in a temporary directory instead of /nix/store. (See: https://github.com/nix-community/lanzaboote/blob/master/rust/lanzatool/src/esp.rs#L56) Great!
RaitoBezarius (Migrated from github.com) reviewed 2023-01-01 19:26:56 +00:00
RaitoBezarius (Migrated from github.com) commented 2023-01-01 19:26:55 +00:00

In that case, I'd just add a warning that it's not exactly a UEFI-spec compliant function, and it's fine IMHO.

In that case, I'd just add a warning that it's not exactly a UEFI-spec compliant function, and it's fine IMHO.
blitz (Migrated from github.com) approved these changes 2023-01-01 23:31:49 +00:00
blitz (Migrated from github.com) left a comment

Love the unit tests!

Love the unit tests!
Sign in to join this conversation.
No description provided.