@ajs124 Thanks for sending this PR.

Could you send a follow-up PR adding a comment about how this dd and mcopy combination works? Why are you skipping the first 2048 bytes with the dd command?

It's actually the first 2048 (512-byte) sectors, as is the default of both dd and most legacy disks:

emily@renko ~/t/m/memtest86-usb.img> sfdisk -l memtest86-usb.img 
Disk memtest86-usb.img: 500 MiB, 524288000 bytes, 1024000 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: gpt
Disk identifier: 68264C0F-858A-49F0-B692-195B64BE4DD7

Device              Start     End Sectors  Size Type
memtest86-usb.img1   2048  512000  509953  249M Microsoft basic data
memtest86-usb.img2 514048 1023966  509919  249M EFI System

So this is skipping the partition table; that means it's actually looking at the basic data partition, not the ESP, but it seems they both contain identical copies of all the files we need. Here's an overengineered alternative:

diff --git a/pkgs/tools/misc/memtest86-efi/default.nix b/pkgs/tools/misc/memtest86-efi/default.nix
index f2adadc0840..3a39ab88ce4 100644
--- a/pkgs/tools/misc/memtest86-efi/default.nix
+++ b/pkgs/tools/misc/memtest86-efi/default.nix
@@ -1,4 +1,4 @@
-{ fetchzip, lib, stdenv, mtools }:
+{ fetchzip, lib, stdenv, mtools, utillinux, jq }:
 
 stdenv.mkDerivation rec {
   pname = "memtest86-efi";
@@ -22,7 +22,7 @@ stdenv.mkDerivation rec {
     stripRoot = false;
   };
 
-  nativeBuildInputs = [ mtools ];
+  nativeBuildInputs = [ mtools utillinux jq ];
 
   installPhase = ''
     mkdir -p $out $TEMP/memtest86-files
@@ -32,7 +32,12 @@ stdenv.mkDerivation rec {
     #
     # The following uses dd and mcopy to extract the actual EFI app from the
     # usb image so that it can be installed directly on the hard drive.
-    dd if=$src/memtest86-usb.img of=$TEMP/ESP.img skip=2048
+    dd if=$src/memtest86-usb.img of=$TEMP/ESP.img $(
+      sfdisk --json memtest86-usb.img | jq -r '.partitiontable |
+        "bs=\(.sectorsize) " +
+        # Select the EFI system partition
+        (.partitions[] | select(.type == "C12A7328-F81F-11D2-BA4B-00A0C93EC93B")
+          | "skip=\(.start) count=\(.size)")')
     mcopy -i $TEMP/ESP.img ::/EFI/BOOT/ $TEMP/memtest86-files/
     mv $TEMP/memtest86-files/BOOT/* $out/
   '';

@emilazy Thanks for describing this.

So this is skipping the partition table; that means it's actually looking at the basic data partition, not the ESP, but it seems they both contain identical copies of all the files we need.

Is this an okay assumption to make?

@emilazy @flokli @ajs124 Without any comments/documentation on this, I'll probably just end up reverting this PR next time I need to update memtest86-efi, unless it "just works".

If it's not a safe assumption to make, then it's easy enough to just select the EFI system partition instead, like my diff does. If people would prefer the additional dependencies of the explicit partition table parsing then I can open a PR for it; I just figured it might be a little excessive.

Please don't just wholesale revert a PR that removed a dependency on an unmaintained, insecure package, though ^^;

@emilazy From my perspective, this derivation that I've been maintaining was updated to use a combination of dd and mtool, but no documentation was added explaining how this works (or why it is safe skipping the first 2048 sectors, or why it is safe to include both partitions in the thing we pass to mtool, or the C12A7328-F81F-11D2-BA4B-00A0C93EC93B string in your diff).

If this doesn't "just work" when I go to update it, and there is no documentation explaining why it works this way, then the easiest thing for me to do is just revert it.

Also, I haven't followed the removal of p7zip, but it seems like somewhat of a stretch to call out its insecurity here? The image memtest86 image here is considered trusted. It should be safe to pass to p7zip.

Unfortunately, your continued insistence that you will unilaterally revert others' changes unless other people do work ahead of time to your precise satisfaction makes me reluctant to invest further effort into contributing to this package, especially as I don't personally use it. I hope my patch and explanations can be useful in making the derivation more future-proof and well-documented, and hereby release them under the CC0 1.0 Universal Public Domain Dedication for the benefit of anyone who wants to use them, but regret that I was unable to convince you to adopt a more collaborative approach to maintenance.

@emilazy I'm sorry to have put you off in this way. I'm guessing it was mostly from the comments about possibly reverting the changes in this PR. I wanted to make it clear that it was a possibility if I had trouble when updating memtest86-efi in the future, but I didn't intend to threaten you (or @ajs124). I appologize for this.

I do appreciate your explanations above, especially #86470 (comment).

I hope you can understand that having good comments in the code is a very important part of the maintainer-ship in nixpkgs. It makes it possible for people to help with things even if they are not super familiar with the derivation in question.

Also, maybe I didn't make this completely clear above, but I would definitely prefer not to revert this. However, without the three things from #86470 (comment) commented clearly in the code, I'm not confident I would be easily able to fix any problems with this derivation in the future.

If updating to a new version of memtest86-efi went smoothly, everything would probably be fine, but if it didn't, I'm not sure how I'd solve the problem. The first thing I'd probably try is reverting this PR (although if p7zip is no longer in nixpkgs, I imagine I'd have to revert even further back, before I was using p7zip).

edit: I should also add that memtest86-efi gets updated pretty rarely--maybe once every couple of months. So there is really no time-pressure for send a PR adding documentation.

@cdepillabout p7zip has been marked as insecure, so it can't be used anymore in a build, also not if the extracted contents are "known"/trusted.

Googling mtools ("a collection of utilities to access MS-DOS disks from GNU and Unix without mounting them"), plus the comment above ("# The following uses dd and mcopy to extract the actual EFI app from the usb image so that it can be installed directly on the hard drive.") was enough for me to understand what was going on - 7zip is able to extract from fat filesystems images (which this package made use of before), this was replaced by using mtools to extract the partition contents (image minus partition table) of that image.

How would you like to see the comment being improved further?

@flokli Thanks for responding here.

I think we may be in the same boat. I also basically understand what is going on.

How would you like to see the comment being improved further?

There are three things in specific I'd like to see in a comment in the code (taken from #86470 (comment)):

• Why is dd skipping the first 2048 sectors? (If you're familiar with Linux at all, it is easy to guess that this is skipping the partition table in the image, but it would be nice to have this in a comment. Otherwise, 2048 looks like a magic number.)
• The output of dd that we are passing to mtool actually contains two separate partitions. Why it is safe to include both partitions in the thing we pass to mtool? Is it documented somewhere that mtool will ignore the second partition? (This is my main concern, and the main thing I'd like to see documented. Although, like I said above, if it is too much work to confirm that mtool does the right thing here in all cases, I'd be fine with just a comment explaining that it appears to work, and a warning for people looking at this in the future that we are relying on this behavior.)
• A short comment about what mtool is doing. I had never seen mtool before. (This is the least important, and like you say, can be figured out from a quick google. I don't think having a one-sentence comment here hurts though.)

@flokli In case you didn't see it, there is a PR from @emilazy adding some of these things to the documentation, and making the derivation a little more robust: #86623.