WCOW Base layer creation and export

Fixes: #853
Implementation of base-layer export and base-layer creation, intended to support FROM scratch non-bootable flows like #750, or supporting the containerd snapshotter in producing mountable, exportable layers which are not based on an existing MS-provided OCI image, so that the containerd snapshotter test suite can pass without significant changes.
It should work with bootable layers, or at least I've confirmed that passing a nanoserver tarball through these two new features produces something functionally-identical to the original nanoserver tarball.
So in theory, this could be used to "squash" several layers into a new single base layer, but I suspect that's not feasible for anyone except MS due to licensing requirements, and based on my learnings here, MS are clearly using a different tool internally to produce current container images.
Things I don't know, and will ignore unless told-otherwise:

Should bcd.bak and related backups be included in the exported tarstream, replacing the modified UtilityVM files they were backups of?
What does lack of a UtilityVM in a base layer actually mean?

Minimal base-layer export is working.
My test case  for this stage is importing mcr.microsoft.com/windows/nanoserver:10.0.19042.630's base (and only) layer using wclayer import, then exporting it to a tarball and importing that tarball to a new directory.
Current differences/issues
No tests
It doesn't appear we have any wclayer tests, beyond some usage for wclayer.CreateScratchLayer on top of nanoserver or busyboxw, relying on Docker to provide the rest of the stack of images.
Certainly nothing testing import/export.
Files that are not round-tripping correctly: Probably working as intended, one open question.
Importing from my generated tarball ends up with different bcd.bak and bcd.log.bak compared to importing from the MS-built tarball. This is because the BCD in the tree is different after untarring, and the bcd.bak is a copy of the bcd from the original tarball.
❔ It's not clear if the exported tarball should invert the backup step, and redirect the BCD and BCD.LOG to use the bcd.bak and bcd.log.bak (etc) files that were saved during import. (Edit: See mutatedFiles and its usage. However, I don't think this is relevant to this PR anyway, as we're working at a lower layer than this.)
A bunch of other files differ, but they differ every time the same tarball is imported anyway.
And really, since my target here is not bootable images but FROM scratch for e.g., #750 or supporting containerd WCOW snapshots sanely, none of the being-changed files matter to me.

Details of differing files
Comparing a containerd import to the wclayer import from the same (MS-generated tarball), the following files differ:
$ diff -ru ../containerd-data/root/io.containerd.snapshotter.v1.windows/snapshots/1 nanoorig/
Binary files ../containerd-data/root/io.containerd.snapshotter.v1.windows/snapshots/1/blank.vhdx and nanoorig/blank.vhdx differ
Binary files ../containerd-data/root/io.containerd.snapshotter.v1.windows/snapshots/1/blank-base.vhdx and nanoorig/blank-base.vhdx differ
Binary files ../containerd-data/root/io.containerd.snapshotter.v1.windows/snapshots/1/UtilityVM/Files/EFI/Microsoft/Boot/BCD and nanoorig/UtilityVM/Files/EFI/Microsoft/Boot/BCD differ
Binary files ../containerd-data/root/io.containerd.snapshotter.v1.windows/snapshots/1/UtilityVM/Files/EFI/Microsoft/Boot/BCD.LOG and nanoorig/UtilityVM/Files/EFI/Microsoft/Boot/BCD.LOG differ
Binary files ../containerd-data/root/io.containerd.snapshotter.v1.windows/snapshots/1/UtilityVM/SystemTemplate.vhdx and nanoorig/UtilityVM/SystemTemplate.vhdx differ
Binary files ../containerd-data/root/io.containerd.snapshotter.v1.windows/snapshots/1/UtilityVM/SystemTemplateBase.vhdx and nanoorig/UtilityVM/SystemTemplateBase.vhdx differ

We know that the BCD files are modified by the import process, and the rest are presumably generated on-the-fly and end up with timestamps inside them or something.
Comparing the wclayer import of the original tarball to the wclayer import of the tarball I generated with wclayer export, the following files differ:
Binary files nanoorig/bcd.bak and newnano/bcd.bak differ
Binary files nanoorig/bcd.log.bak and newnano/bcd.log.bak differ
Binary files nanoorig/blank.vhdx and newnano/blank.vhdx differ
Binary files nanoorig/blank-base.vhdx and newnano/blank-base.vhdx differ
Binary files nanoorig/UtilityVM/Files/EFI/Microsoft/Boot/BCD and newnano/UtilityVM/Files/EFI/Microsoft/Boot/BCD differ
Binary files nanoorig/UtilityVM/Files/EFI/Microsoft/Boot/BCD.LOG and newnano/UtilityVM/Files/EFI/Microsoft/Boot/BCD.LOG differ
Binary files nanoorig/UtilityVM/SystemTemplate.vhdx and newnano/UtilityVM/SystemTemplate.vhdx differ
Binary files nanoorig/UtilityVM/SystemTemplateBase.vhdx and newnano/UtilityVM/SystemTemplateBase.vhdx differ

And nailing down what all the BCD files are
$ sha256sum */bcd.bak */UtilityVM/Files/EFI/Microsoft/Boot/BCD
71b8db95fdc7339e00bc6ac87505d5cf21987fcdd171184a88e91836a884a83b *nanoorig/bcd.bak
6d6597ec802a43cb9b53b3bddb2e6eccd806abc2871cbb454495a3e38a087227 *newnano/bcd.bak
6d6597ec802a43cb9b53b3bddb2e6eccd806abc2871cbb454495a3e38a087227 *newnano2/bcd.bak
6d6597ec802a43cb9b53b3bddb2e6eccd806abc2871cbb454495a3e38a087227 *nanoorig/UtilityVM/Files/EFI/Microsoft/Boot/BCD
3546633c9b234e14b30fc06b24fd0757a474b36771382764db7db9f6a0de3a7e *newnano/UtilityVM/Files/EFI/Microsoft/Boot/BCD
f084cc86c3b06a96e497d354003b0436a6210b4412c26cf65f5bff3d262b6764 *newnano2/UtilityVM/Files/EFI/Microsoft/Boot/BCD

We can see that the bcd.bak that comes from importing my generated tarball is the BCD from the from-MS imported Utility VM; bcd.bak in the from-MS import is presumably the BCD that's actually inside the tarball.
This is an oddity of ociwriter.ImportLayer. It backs up BCD and BCD.LOG*, but doesn't do anything with those backups, they are not referenced anywhere else in the hcsshim code base. I'm not sure if the intent was to copy them back after importing, or if they are supposed to be included in the export (i.e. invert the backup process).

File order is different: Only annoying when doing comparisons of the tar tv output.
I don't know what tool is being used to create the files published by MS, but clearly it's not filepath.Walk in Go, as they disagree about the ordering of case. I suspect the most-important thing here is that Files comes before UtilityVM. Order of processing files will affect hard-links in the tarball, but only cosmetically.
Hard links: Works for base layers, but not non-base layers (but the latter is unrelated to this PR).
Resolved, for this use-case. The layer export code now recognises hard-links on the filesystem, and adds them to the tar stream. The algorithm roughly matches the one given at the OCI Image Spec "Image Layer Filesystem Changeset" notes on hard-links.
The hard-links present in the nanoserver image are also present if that layer is reexported, but:

FIle order means a different file might be "the file" in the tarball, because the data is recorded against the first-seen name.
Somehow the nanoserver tarball from mcr contains hardlinks with different modtimes to their targets (observed in links from UtilityVM/Files/ to Files/). That information is lost when importing the layer (those details are a factor of the single file, not the multiple directory entries linking to it) so that's not possible to replicate. Nor desirable, I think. I'm not sure how that is possible in the first place, short of stale directory entries on the source system when creating the tarball.

In testing, I noticed that hard-links that exist in a non-base layer do not appear as hard-links when exporting, because modvmcompute.NewProc("ExportLayer") does not preserve hard links when copying the layer data into a temporary directory. It does not create BACKUP_LINK streams, and even if it did, go-winio's backuptar doesn't handle them anyway. It's not obvious if the BACKUP_LINK data format is well-defined but not documented, or is supposed to be application-defined, in which case the content would have to be part of ExportLayer's API and somehow make sense both intra-layer and inter-layer.
I also found that New-Item -ItemType HardLink didn't work to create cross-layer links (which makes sense, since I assume it wants to change the 'link count' on a file in a read-only layer), so I suspect that at this time, hcsshim will never export hard-links except when exporting a base layer (i.e. this new code). Maybe this used to work, since the code exists to handle both inter-layer and intra-layer hard links in tarballs during import.
Hard-link support also pulls in a fairly noisy Microsoft/go-winio bump which will resolvably conflict with #941, and since I was running go mod tidy as a matter of course, I think I inlined #942 in passing. If the latter lands first, this'll be less noisy, but the go-winio bump will still generate noise in both modules because it has bumped golang.org/x/sys quite a long way, which flows through into the vendor directory. (This was broken out into separate PRs)

Notes from before this was implemented
The tarball from MS for nanoserver's base layer includes lots of hardlinks, some between entries in Files, but most are from the UtilityVM/Files back to Files/. Not capturing these hard links when exporting the layer takes an uncompressed nanoserver base layer from about 280MB to about 430MB.
This one's a little intricate. winio.NewBackupFileReader doesn't generate winio.BackupLink, that's up to the code walking the tree, and I haven't yet worked out how, from Go, to actually identify that a file is a HardLink, which PowerShell can tell me, and track down the other links, which fsutil hardlinks can tell me. So the data's there, but the API has escaped my minimal searching. I am guessing I want BY_HANDLE_FILE_INFORMATION to tell me when something has hard-links, and match them by file index.
Dagnabbit: Go's os.File.Stat actually fetches exactly this structure, but doesn't grab the number of links, and the index values are not exported. So we'll be making syscalls of our own to achieve this. os.SameFile uses the indexes, but I can't see a good way to apply that here.
Also, github.com/Microsoft/go-winio/backuptar.WriteTarFileFromBackupStream doesn't handle winio.BackupLink anyway. So that would have to be implemented too, or intercept the call to backuptar.WriteTarFileFromBackupStream in ociwclayer.writeTarFromLayer like we do for deleted files. That's probably necessary anyway, as per the MS docs, the actual data in the winio.BackupLink object is up to the application. It would also mirror ociwclayer.writeLayerFromTar which similarly intercepts the call to writeBackupStreamFromTarAndSaveMutatedFiles for tar.TypeLink headers.
I did a quick test of just tarballing-up the tree using Windows-bundled tar, and it did find hardlinks correctly, although it disagreed about which one in the tarball was the "original", not that it matters. It also failed to include UtilityVM but did include UtilityVM\Files, but that could probably be fixed with a better command-line and more than the zero effort I put into this test:
tar cf simplenano.tar -C .\nanoorig Files UtilityVM\Files

No permission bits on the exported tarballs: This was already the case for the non-base layers.
backuptar.WriteTarFileFromBackupStream just doesn't have any way to set the permission bits on the tar headers it creates. I considered making it automatically apply 0644 for files and 0755 for directories to match the MS-generated tarballs, but (a) that's way over in go-winio, and is separate to this; and (b) I'm only 80% sure that won't invalidate every Windows Containers build-cache on the planet, and I don't think 20% is low-enough to risk having to change my name and go into hiding before an angry mob of CI/CD engineers comes looking for me.
Weirdly, the MS-built tarball doesn't have permission bits on the Files, UtilityVM, or UtilityVM/Files directories, which suggests that whatever is done to generate these tarballs, it's doing something fairly specialised and a bit unusual. And also suggests this isn't important.
Punted to https://github.com/microsoft/go-winio/issues/184 for discussion/feedback.

Creating a base layer from some files on-disk is working.
This introduces a new API, hcsshim.ConvertToBaseLayer:

If any of the below files are missing, create them as empty files so hcsshim.ProcessBaseLayer will complete:

Files/Windows/System32/config/DEFAULT

Files/Windows/System32/config/SAM

Files/Windows/System32/config/SECURITY

Files/Windows/System32/config/SOFTWARE

Files/Windows/System32/config/SYSTEM

Checks that if UtilityVM/Files exists, it contains UtilityVM/Files/EFI/Microsoft/Boot/BCD so that hcschim.ProcessUtilityVMImage will complete (assuming the BCD is valid).

It might be possible to create a blank BCD file but I think that's out-of-scope for hcsshim.

Runs ProcessBaseLayer and if necessary, ProcessUtilityVMImage

Basically, this is the part of (w *baseLayerWriter) Close() after reapplyDirectoryTimes.

I'm not sure when a UtilityVM is necessary. At the moment, I'm assuming it's only needed if the layer is used as the base of a container's scratch layer, and that's not the target use-case for this work.
As it happens, the test TestSCSIAddRemoveWCOW is a passable candidate for exercising this API and particularly the use-case I'm targeting, as it creates scratch layers for attaching to a container as SCSI disks. Before this change, those scratch layers are based on the image used to create the container in the first place, i.e. nanoserver. With this API, it is much more similar to TestSCSIAddRemoveLCOW which uses trivial empty EXT4 scratch layers.
All of the above could be handled by the hcsshim user, e.g., this was done (but never merged) by refactoring the containerd snapshot testsuite in https://github.com/containerd/containerd/pull/2366 to use hcsshim.ProcessBaseLayer and fstest.Base, including defining fstest.Base in containerd/continuity in order to record an undocumented internal hcsshim requirement.
I propose instead to make containerd's snapshot Commit API call hcsshim.ConvertToBaseLayer at the appropriate point so that to callers of the snapshot system, no behaviour difference is seen between Windows and non-Windows behaviours.
My fallback is to take all the code I wrote here, and add it to containerd instead, which of course means no one else benefits, and the knowledge needed to call hcsshim.Process* remains buried over in some non-hcsshim project.
Current differences/issues
Test-case TODO question
testutilities.CreateWCOWBlankRWLayer has a TODO:
// TODO: This is wrong. Need to search the folders.

It appears that it intends to search the provided layers to find one with a UtilityVM folder, but it's not clear why, as its current use-case is for scratch layers attached as SCSI devices via the Utility VM, which does not, as far as I can see, require a Utility VM.
It's a TODO here because before this change, the top layer has a Utility VM, but with this change, it does not.
The test passes so I think I'm okay, but it's possible there's a downlevel issue, as I'm testing on Windows 10 20H2.
cd tests
go test -v -tags "uvmscsi" -run TestSCSIAddRemoveWCOW  ./...

There is no step 3
Well, there is, but not here. Assuming that this work is completed and merged, I need to revendor into containerd, and rebase my work there (see https://github.com/containerd/containerd/pull/4415#discussion_r532221174) to take advantage of this new API, and hopefully get the snapshot integration test suite to start passing, which creates arbitrary base layers in every test.

Per discussion in #916, the new API hcsshim.ConvertToBaseLayer might want to be exposed via pkgs instead. Although perhaps that is true of the entire layer.go API (which is already just thin bounces into internal/ anyway.

Per discussion in #916, the new API hcsshim.ConvertToBaseLayer might want to be exposed via pkgs instead. Although perhaps that is true of the entire layer.go API (which is already just thin bounces into internal/ anyway.

Another API thought. Since this is currently being added to layer.go where almost all the APIs take info DriverInfo, layerId string rather than path string, should ConvertToBaseLayer be the same?
I didn't do this initially because the two current exceptions to this API structure are the functions the new API wraps, ProcessBaseLayer and ProcessUtilityVMImage.

Another API thought. Since this is currently being added to layer.go where almost all the APIs take info DriverInfo, layerId string rather than path string, should ConvertToBaseLayer be the same?
I didn't do this initially because the two current exceptions to this API structure are the functions the new API wraps, ProcessBaseLayer and ProcessUtilityVMImage.

Weirdly, even though it's passed on the containerd CI in the past, I'm finding on my local machine that a base layer I create this way fails if I try to import a layer based on it, with
    wcow_layers_test.go:168: C:\Users\paulh\AppData\Local\Temp\test492934055\layer-1: hcsshim::ImportLayer - failed failed in Win32: The configuration registry database is corrupt. (0x3f1)

Inside the fold is the test-case, basically me attempting to reproduce the 128Layers test from containerd that seems to hit some race condition on their CI, but is instead suggesting the new ConvertToBaseLayer API I'm adding here is incorrect.
It might also be that it works on the Windows Server RS5 CI host, but not on my Windows 10 20H2 desktop machine. -_-
And to reverse that, if I use a nanoserver image instead of creating my own in ConvertToBaseLayer, my 128Layer test passes on my desktop machine. So either something is significantly different in the OS, or I've mistranscribed the 128Layer test, or the containerd Snapshot system is doing something else I haven't identified yet.
I should try and run the containerd 128Layer test on my desktop, and see if that passes consistently, which would suggest some bug in HCS which has been fixed since Windows Server RS5. However, I expect it will fail with the same issue I started this comment with, since it's also using ConvertToBaseLayer to create a new base layer in that test.

tests/functional/wcow_layers_test.go
// +build windows

package functional

import (
	"context"
	"fmt"
	"io"
	"io/ioutil"
	"os"
	"path/filepath"
	"strings"
	"testing"

	"github.com/pkg/errors"
	"go.opencensus.io/trace"
	"golang.org/x/sys/windows"

	layerspkg "github.com/Microsoft/hcsshim/internal/layers"
	"github.com/Microsoft/hcsshim/internal/oc"
	"github.com/Microsoft/hcsshim/internal/wclayer"
	"github.com/Microsoft/hcsshim/pkg/ociwclayer"
	testutilities "github.com/Microsoft/hcsshim/test/functional/utilities"
)

// Initially, just an attempt to recreate the failure of the 128LayersMount test
// (and similar failures) in the containerd Snapshot Testsuite.
// https://github.com/containerd/containerd/pull/4419#issuecomment-762830024

// The rough logic of 128layers is:
// Create a base layer:
/*
	fstest.CreateFile("/bottom", []byte("way at the bottom\n"), 0777),
	fstest.CreateFile("/overwriteme", []byte("FIRST!\n"), 0777),
	fstest.CreateDir("/addhere", 0755),
	fstest.CreateDir("/onlyme", 0755),
	fstest.CreateFile("/onlyme/bottom", []byte("bye!\n"), 0777),
*/
// Then for 1..127, the layer is mounted, the below done, then the layer is
// unmounted and turned into a base layer for the next iteration.
/*
	fstest.CreateFile("/overwriteme", []byte(fmt.Sprintf("%d WAS HERE!\n", i)), 0777),
	fstest.CreateFile(fmt.Sprintf("/addhere/file-%d", i), []byte("same\n"), 0755),
	fstest.RemoveAll("/onlyme"),
	fstest.CreateDir("/onlyme", 0755),
	fstest.CreateFile(fmt.Sprintf("/onlyme/file-%d", i), []byte("only me!\n"), 0777),
*/

// Annoyingly, "Turn a scratch layer into a RO layer" lives in containerd, not hcsshim.
// Thankfully, on this branch, I _do_ have "Turn random directory into Base RO layer"

func createFile(t *testing.T, root, path string, data []byte) {
	targetPath := filepath.Join(root, path)

	if file, err := os.Create(targetPath); err != nil {
		t.Fatal(err)
	} else {
		if _, err = file.Write(data); err != nil {
			t.Fatal(err)
		}

		if err = file.Close(); err != nil {
			t.Fatal(err)
		}
	}
}

func createDir(t *testing.T, root, path string) {
	targetPath := filepath.Join(root, path)

	if err := os.MkdirAll(targetPath, 0); err != nil {
		t.Fatal(err)
	}
}

func removeAll(t *testing.T, root, path string) {
	targetPath := filepath.Join(root, path)

	if err := os.RemoveAll(targetPath); err != nil {
		t.Fatal(err)
	}
}

func mountLayer(t *testing.T, layerStack []string) string {
	volumePath, err := layerspkg.MountContainerLayers(context.Background(), layerStack, "", nil)
	if err != nil {
		t.Fatal(err)
	}
	return volumePath
}

func unmountLayer(t *testing.T, layerStack []string) {
	if err := layerspkg.UnmountContainerLayers(context.Background(), layerStack, "", nil, layerspkg.UnmountOperationAll); err != nil {
		t.Fatal(err)
	}
}

func createScratch(t *testing.T, layerStack []string) {
	path := layerStack[len(layerStack)-1]
	parentLayerPaths := layerStack[:len(layerStack)-1]

	if err := wclayer.CreateScratchLayer(context.Background(), path, parentLayerPaths); err != nil {
		t.Fatal(err)
	}

}

// Mount volumePath (in format '\\?\Volume{GUID}' at targetPath.
// https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setvolumemountpointw
func setVolumeMountPoint(targetPath string, volumePath string) error {
	if !strings.HasPrefix(volumePath, "\\\\?\\Volume{") {
		return errors.Errorf("unable to mount non-volume path %s", volumePath)
	}

	// Both must end in a backslash
	slashedTarget := filepath.Clean(targetPath) + string(filepath.Separator)
	slashedVolume := volumePath + string(filepath.Separator)

	targetP, err := windows.UTF16PtrFromString(slashedTarget)
	if err != nil {
		return errors.Wrapf(err, "unable to utf16-ise %s", slashedTarget)
	}

	volumeP, err := windows.UTF16PtrFromString(slashedVolume)
	if err != nil {
		return errors.Wrapf(err, "unable to utf16-ise %s", slashedVolume)
	}

	if err := windows.SetVolumeMountPoint(targetP, volumeP); err != nil {
		return errors.Wrapf(err, "failed calling SetVolumeMount('%s', '%s')", slashedTarget, slashedVolume)
	}

	return nil
}

// Remove the volume mount at targetPath
// https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-deletevolumemountpointa
func deleteVolumeMountPoint(targetPath string) error {
	// Must end in a backslash
	slashedTarget := filepath.Clean(targetPath) + string(filepath.Separator)

	targetP, err := windows.UTF16PtrFromString(slashedTarget)
	if err != nil {
		return errors.Wrapf(err, "unable to utf16-ise %s", slashedTarget)
	}

	if err := windows.DeleteVolumeMountPoint(targetP); err != nil {
		return errors.Wrapf(err, "failed calling DeleteVolumeMountPoint('%s')", slashedTarget)
	}

	return nil
}

func restreamLayer(t *testing.T, layerFolders []string) {
	ctx := context.Background()

	path := layerFolders[len(layerFolders)-1]
	parentLayerPaths := layerFolders[:len(layerFolders)-1]

	reader, writer := io.Pipe()

	go func() {
		err := ociwclayer.ExportLayerToTar(ctx, writer, path, parentLayerPaths)
		writer.CloseWithError(err)
	}()

	if _, err := ociwclayer.ImportLayerFromTar(ctx, reader, path, parentLayerPaths); err != nil {
		t.Fatalf("%s: %v", path, err)
	}

	if _, err := io.Copy(ioutil.Discard, reader); err != nil {
		t.Fatalf("%s: %v", path, err)
	}
}

func reverse(a []string) {
	for i := len(a)/2 - 1; i >= 0; i-- {
		opp := len(a) - 1 - i
		a[i], a[opp] = a[opp], a[i]
	}
}

func Test128Layers(t *testing.T) {
	trace.ApplyConfig(trace.Config{DefaultSampler: trace.AlwaysSample()})
	trace.RegisterExporter(&oc.LogrusExporter{})

	// Get temp dir, and create base-dir with Files/ directory
	tempDir := testutilities.CreateTempDir(t)
	defer func() {
		if err := os.RemoveAll(tempDir); err != nil {
			t.Fatal(err)
		}
	}()

	baseDir := filepath.Join(tempDir, "layer-0")
	/* This has stopped working, it's refusing to restream the upper layers... */
	if err := os.MkdirAll(filepath.Join(baseDir, "Files"), 0); err != nil {
		t.Fatal(err)
	}

	// Apply the above filesystem changes
	createFile(t, filepath.Join(baseDir, "Files"), "bottom", []byte("way at the bottom\n"))
	createFile(t, filepath.Join(baseDir, "Files"), "overwriteme", []byte("FIRST!\n"))
	createDir(t, filepath.Join(baseDir, "Files"), "addhere")
	createDir(t, filepath.Join(baseDir, "Files"), "onlyme")
	createFile(t, filepath.Join(baseDir, "Files"), filepath.Join("onlyme", "bottom"), []byte("bye!\n"))

	// Turn it into a Base Layer
	if err := wclayer.ConvertToBaseLayer(context.Background(), baseDir); err != nil {
		t.Fatal(err)
	}
	defer func() {
		if err := wclayer.DestroyLayer(context.Background(), baseDir); err != nil {
			t.Fatal(err)
		}
	}()

	layers := []string{baseDir}
	/* And skip the below */
	/* Use a copy of nanoserver I have lying around

	nanoDir := "C:\\Users\\paulh\\tt\\nano"
	layers := []string{nanoDir}

	layers = append(layers, baseDir)
	createScratch(t, layers)
	defer func() {
		if err := wclayer.DestroyLayer(context.Background(), baseDir); err != nil {
			t.Fatal(err)
		}
	}()

	volumePath := mountLayer(t, layers)

	mountPath := filepath.Join(tempDir, "mount-0")
	if err := os.MkdirAll(mountPath, 0); err != nil {
		t.Fatal(err)
	}

	if err := setVolumeMountPoint(mountPath, volumePath); err != nil {
		t.Fatal(err)
	}

	createFile(t, mountPath, "bottom", []byte("way at the bottom\n"))
	createFile(t, mountPath, "overwriteme", []byte("FIRST!\n"))
	createDir(t, mountPath, "addhere")
	createDir(t, mountPath, "onlyme")
	createFile(t, mountPath, filepath.Join("onlyme", "bottom"), []byte("bye!\n"))

	// Unmount the scratch
	if err := deleteVolumeMountPoint(mountPath); err != nil {
		t.Fatal(err)
	}

	if err := os.Remove(mountPath); err != nil {
		t.Fatal(err)
	}

	unmountLayer(t, layers)

	// Restream it as a read-only layer
	restreamLayer(t, layers)

	*/
	// Back to your regularly-scheduled programming

	// Now loop:
	for i := 1; i <= 127; i++ {
		scratchDir := filepath.Join(tempDir, fmt.Sprintf("layer-%d", i))

		// Mount a scratch on our list
		layers = append(layers, scratchDir)
		reverse(layers[:len(layers)-1])
		createScratch(t, layers)
		defer func() {
			if err := wclayer.DestroyLayer(context.Background(), scratchDir); err != nil {
				t.Fatal(err)
			}
		}()

		volumePath := mountLayer(t, layers)

		mountPath := filepath.Join(tempDir, fmt.Sprintf("mount-%d", i))
		if err := os.MkdirAll(mountPath, 0); err != nil {
			t.Fatal(err)
		}

		if err := setVolumeMountPoint(mountPath, volumePath); err != nil {
			t.Fatal(err)
		}

		// Perform the changes
		createFile(t, mountPath, "overwriteme", []byte(fmt.Sprintf("%d WAS HERE!\n", i)))
		createFile(t, mountPath, filepath.Join("addhere", fmt.Sprintf("file-%d", i)), []byte("same\n"))
		removeAll(t, mountPath, "onlyme")
		createDir(t, mountPath, "onlyme")
		createFile(t, mountPath, filepath.Join("onlyme", fmt.Sprintf("file-%d", i)), []byte("only me!\n"))

		// Unmount the scratch
		if err := deleteVolumeMountPoint(mountPath); err != nil {
			t.Fatal(err)
		}

		if err := os.Remove(mountPath); err != nil {
			t.Fatal(err)
		}

		unmountLayer(t, layers)

		// Restream it as a read-only layer
		restreamLayer(t, layers)
		reverse(layers[:len(layers)-1])
	}

}

Weirdly, even though it's passed on the containerd CI in the past, I'm finding on my local machine that a base layer I create this way fails if I try to import a layer based on it, with
    wcow_layers_test.go:168: C:\Users\paulh\AppData\Local\Temp\test492934055\layer-1: hcsshim::ImportLayer - failed failed in Win32: The configuration registry database is corrupt. (0x3f1)

Inside the fold is the test-case, basically me attempting to reproduce the 128Layers test from containerd that seems to hit some race condition on their CI, but is instead suggesting the new ConvertToBaseLayer API I'm adding here is incorrect.
It might also be that it works on the Windows Server RS5 CI host, but not on my Windows 10 20H2 desktop machine. -_-
And to reverse that, if I use a nanoserver image instead of creating my own in ConvertToBaseLayer, my 128Layer test passes on my desktop machine. So either something is significantly different in the OS, or I've mistranscribed the 128Layer test, or the containerd Snapshot system is doing something else I haven't identified yet.
I should try and run the containerd 128Layer test on my desktop, and see if that passes consistently, which would suggest some bug in HCS which has been fixed since Windows Server RS5. However, I expect it will fail with the same issue I started this comment with, since it's also using ConvertToBaseLayer to create a new base layer in that test.

tests/functional/wcow_layers_test.go
// +build windows

package functional

import (
	"context"
	"fmt"
	"io"
	"io/ioutil"
	"os"
	"path/filepath"
	"strings"
	"testing"

	"github.com/pkg/errors"
	"go.opencensus.io/trace"
	"golang.org/x/sys/windows"

	layerspkg "github.com/Microsoft/hcsshim/internal/layers"
	"github.com/Microsoft/hcsshim/internal/oc"
	"github.com/Microsoft/hcsshim/internal/wclayer"
	"github.com/Microsoft/hcsshim/pkg/ociwclayer"
	testutilities "github.com/Microsoft/hcsshim/test/functional/utilities"
)

// Initially, just an attempt to recreate the failure of the 128LayersMount test
// (and similar failures) in the containerd Snapshot Testsuite.
// https://github.com/containerd/containerd/pull/4419#issuecomment-762830024

// The rough logic of 128layers is:
// Create a base layer:
/*
	fstest.CreateFile("/bottom", []byte("way at the bottom\n"), 0777),
	fstest.CreateFile("/overwriteme", []byte("FIRST!\n"), 0777),
	fstest.CreateDir("/addhere", 0755),
	fstest.CreateDir("/onlyme", 0755),
	fstest.CreateFile("/onlyme/bottom", []byte("bye!\n"), 0777),
*/
// Then for 1..127, the layer is mounted, the below done, then the layer is
// unmounted and turned into a base layer for the next iteration.
/*
	fstest.CreateFile("/overwriteme", []byte(fmt.Sprintf("%d WAS HERE!\n", i)), 0777),
	fstest.CreateFile(fmt.Sprintf("/addhere/file-%d", i), []byte("same\n"), 0755),
	fstest.RemoveAll("/onlyme"),
	fstest.CreateDir("/onlyme", 0755),
	fstest.CreateFile(fmt.Sprintf("/onlyme/file-%d", i), []byte("only me!\n"), 0777),
*/

// Annoyingly, "Turn a scratch layer into a RO layer" lives in containerd, not hcsshim.
// Thankfully, on this branch, I _do_ have "Turn random directory into Base RO layer"

func createFile(t *testing.T, root, path string, data []byte) {
	targetPath := filepath.Join(root, path)

	if file, err := os.Create(targetPath); err != nil {
		t.Fatal(err)
	} else {
		if _, err = file.Write(data); err != nil {
			t.Fatal(err)
		}

		if err = file.Close(); err != nil {
			t.Fatal(err)
		}
	}
}

func createDir(t *testing.T, root, path string) {
	targetPath := filepath.Join(root, path)

	if err := os.MkdirAll(targetPath, 0); err != nil {
		t.Fatal(err)
	}
}

func removeAll(t *testing.T, root, path string) {
	targetPath := filepath.Join(root, path)

	if err := os.RemoveAll(targetPath); err != nil {
		t.Fatal(err)
	}
}

func mountLayer(t *testing.T, layerStack []string) string {
	volumePath, err := layerspkg.MountContainerLayers(context.Background(), layerStack, "", nil)
	if err != nil {
		t.Fatal(err)
	}
	return volumePath
}

func unmountLayer(t *testing.T, layerStack []string) {
	if err := layerspkg.UnmountContainerLayers(context.Background(), layerStack, "", nil, layerspkg.UnmountOperationAll); err != nil {
		t.Fatal(err)
	}
}

func createScratch(t *testing.T, layerStack []string) {
	path := layerStack[len(layerStack)-1]
	parentLayerPaths := layerStack[:len(layerStack)-1]

	if err := wclayer.CreateScratchLayer(context.Background(), path, parentLayerPaths); err != nil {
		t.Fatal(err)
	}

}

// Mount volumePath (in format '\\?\Volume{GUID}' at targetPath.
// https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setvolumemountpointw
func setVolumeMountPoint(targetPath string, volumePath string) error {
	if !strings.HasPrefix(volumePath, "\\\\?\\Volume{") {
		return errors.Errorf("unable to mount non-volume path %s", volumePath)
	}

	// Both must end in a backslash
	slashedTarget := filepath.Clean(targetPath) + string(filepath.Separator)
	slashedVolume := volumePath + string(filepath.Separator)

	targetP, err := windows.UTF16PtrFromString(slashedTarget)
	if err != nil {
		return errors.Wrapf(err, "unable to utf16-ise %s", slashedTarget)
	}

	volumeP, err := windows.UTF16PtrFromString(slashedVolume)
	if err != nil {
		return errors.Wrapf(err, "unable to utf16-ise %s", slashedVolume)
	}

	if err := windows.SetVolumeMountPoint(targetP, volumeP); err != nil {
		return errors.Wrapf(err, "failed calling SetVolumeMount('%s', '%s')", slashedTarget, slashedVolume)
	}

	return nil
}

// Remove the volume mount at targetPath
// https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-deletevolumemountpointa
func deleteVolumeMountPoint(targetPath string) error {
	// Must end in a backslash
	slashedTarget := filepath.Clean(targetPath) + string(filepath.Separator)

	targetP, err := windows.UTF16PtrFromString(slashedTarget)
	if err != nil {
		return errors.Wrapf(err, "unable to utf16-ise %s", slashedTarget)
	}

	if err := windows.DeleteVolumeMountPoint(targetP); err != nil {
		return errors.Wrapf(err, "failed calling DeleteVolumeMountPoint('%s')", slashedTarget)
	}

	return nil
}

func restreamLayer(t *testing.T, layerFolders []string) {
	ctx := context.Background()

	path := layerFolders[len(layerFolders)-1]
	parentLayerPaths := layerFolders[:len(layerFolders)-1]

	reader, writer := io.Pipe()

	go func() {
		err := ociwclayer.ExportLayerToTar(ctx, writer, path, parentLayerPaths)
		writer.CloseWithError(err)
	}()

	if _, err := ociwclayer.ImportLayerFromTar(ctx, reader, path, parentLayerPaths); err != nil {
		t.Fatalf("%s: %v", path, err)
	}

	if _, err := io.Copy(ioutil.Discard, reader); err != nil {
		t.Fatalf("%s: %v", path, err)
	}
}

func reverse(a []string) {
	for i := len(a)/2 - 1; i >= 0; i-- {
		opp := len(a) - 1 - i
		a[i], a[opp] = a[opp], a[i]
	}
}

func Test128Layers(t *testing.T) {
	trace.ApplyConfig(trace.Config{DefaultSampler: trace.AlwaysSample()})
	trace.RegisterExporter(&oc.LogrusExporter{})

	// Get temp dir, and create base-dir with Files/ directory
	tempDir := testutilities.CreateTempDir(t)
	defer func() {
		if err := os.RemoveAll(tempDir); err != nil {
			t.Fatal(err)
		}
	}()

	baseDir := filepath.Join(tempDir, "layer-0")
	/* This has stopped working, it's refusing to restream the upper layers... */
	if err := os.MkdirAll(filepath.Join(baseDir, "Files"), 0); err != nil {
		t.Fatal(err)
	}

	// Apply the above filesystem changes
	createFile(t, filepath.Join(baseDir, "Files"), "bottom", []byte("way at the bottom\n"))
	createFile(t, filepath.Join(baseDir, "Files"), "overwriteme", []byte("FIRST!\n"))
	createDir(t, filepath.Join(baseDir, "Files"), "addhere")
	createDir(t, filepath.Join(baseDir, "Files"), "onlyme")
	createFile(t, filepath.Join(baseDir, "Files"), filepath.Join("onlyme", "bottom"), []byte("bye!\n"))

	// Turn it into a Base Layer
	if err := wclayer.ConvertToBaseLayer(context.Background(), baseDir); err != nil {
		t.Fatal(err)
	}
	defer func() {
		if err := wclayer.DestroyLayer(context.Background(), baseDir); err != nil {
			t.Fatal(err)
		}
	}()

	layers := []string{baseDir}
	/* And skip the below */
	/* Use a copy of nanoserver I have lying around

	nanoDir := "C:\\Users\\paulh\\tt\\nano"
	layers := []string{nanoDir}

	layers = append(layers, baseDir)
	createScratch(t, layers)
	defer func() {
		if err := wclayer.DestroyLayer(context.Background(), baseDir); err != nil {
			t.Fatal(err)
		}
	}()

	volumePath := mountLayer(t, layers)

	mountPath := filepath.Join(tempDir, "mount-0")
	if err := os.MkdirAll(mountPath, 0); err != nil {
		t.Fatal(err)
	}

	if err := setVolumeMountPoint(mountPath, volumePath); err != nil {
		t.Fatal(err)
	}

	createFile(t, mountPath, "bottom", []byte("way at the bottom\n"))
	createFile(t, mountPath, "overwriteme", []byte("FIRST!\n"))
	createDir(t, mountPath, "addhere")
	createDir(t, mountPath, "onlyme")
	createFile(t, mountPath, filepath.Join("onlyme", "bottom"), []byte("bye!\n"))

	// Unmount the scratch
	if err := deleteVolumeMountPoint(mountPath); err != nil {
		t.Fatal(err)
	}

	if err := os.Remove(mountPath); err != nil {
		t.Fatal(err)
	}

	unmountLayer(t, layers)

	// Restream it as a read-only layer
	restreamLayer(t, layers)

	*/
	// Back to your regularly-scheduled programming

	// Now loop:
	for i := 1; i <= 127; i++ {
		scratchDir := filepath.Join(tempDir, fmt.Sprintf("layer-%d", i))

		// Mount a scratch on our list
		layers = append(layers, scratchDir)
		reverse(layers[:len(layers)-1])
		createScratch(t, layers)
		defer func() {
			if err := wclayer.DestroyLayer(context.Background(), scratchDir); err != nil {
				t.Fatal(err)
			}
		}()

		volumePath := mountLayer(t, layers)

		mountPath := filepath.Join(tempDir, fmt.Sprintf("mount-%d", i))
		if err := os.MkdirAll(mountPath, 0); err != nil {
			t.Fatal(err)
		}

		if err := setVolumeMountPoint(mountPath, volumePath); err != nil {
			t.Fatal(err)
		}

		// Perform the changes
		createFile(t, mountPath, "overwriteme", []byte(fmt.Sprintf("%d WAS HERE!\n", i)))
		createFile(t, mountPath, filepath.Join("addhere", fmt.Sprintf("file-%d", i)), []byte("same\n"))
		removeAll(t, mountPath, "onlyme")
		createDir(t, mountPath, "onlyme")
		createFile(t, mountPath, filepath.Join("onlyme", fmt.Sprintf("file-%d", i)), []byte("only me!\n"))

		// Unmount the scratch
		if err := deleteVolumeMountPoint(mountPath); err != nil {
			t.Fatal(err)
		}

		if err := os.Remove(mountPath); err != nil {
			t.Fatal(err)
		}

		unmountLayer(t, layers)

		// Restream it as a read-only layer
		restreamLayer(t, layers)
		reverse(layers[:len(layers)-1])
	}

}

Converting to Draft because there's definitely something OS-version-specific going on. The 128 Layers test passes on whatever AppVeyor is running (it doesn't actually say), and until now, I had not realised that the functional tests are not run by the CI pipeline, so I have a hack-commit here to run the 128Layers hack-test, in case I am able to work out what I need to do to generate a valid base layer under Windows 20H2.

Converting to Draft because there's definitely something OS-version-specific going on. The 128 Layers test passes on whatever AppVeyor is running (it doesn't actually say), and until now, I had not realised that the functional tests are not run by the CI pipeline, so I have a hack-commit here to run the 128Layers hack-test, in case I am able to work out what I need to do to generate a valid base layer under Windows 20H2.

@TBBle Just to make sure I understand this, the hope here is the ability to make any arbitrary directory a valid base layer (at least one that can be pushed to a registry, it won't be bootable obviously) by explicitly adding in what the platform expects (hive files etc.)?

Yeah, that's exactly what I'm looking for. Effectively, I want something like WCOW FROM scratch, knowing that it won't be bootable.
I was inspired by some discussion about job containers and "slim base containers" in the context of Kubernetes WCOW privileged containers, but the motivations for me are both file distribution, e.g. #750, and for BuildKit, which puts its context and Dockerfile into parentless containerd snapshots.

@TBBle Awesome, ok we're on the same page here. I'm hoping to give this a review within the next week (unless your hunch is things will change dramatically to fix the 128 layer shenanigans)

I don't expect dramatic changes. The code as-is works on Windows RS5; feel free to ignore the top two commits, I'm kind-of waiting on having running integration test-suite in CI (which #936 suggests might be waiting for a move to GitHub Actions) before I clean up and make that hack-test a real test.
So if we're happy to merge in the works-with-RS5-but-fails-for-some-use-cases-with-20H2 state, then I can just peal off the top two hack-commits and we can review this.
I'm wrong and something changed post-RS5 that requires significant changes, hopefully we can work that out before Windows Server LTSC 2021 (guessing the name) happens, so we don't end up being a hard-blocker on k8s or containerd.
I have a couple of paths of exploration (mentioned above) to resolve this with 20H2, but haven't had the free time (this is a personal project, not work-supported) to actually try them out.

So as well as stashing those two hack-commits for the 128Layers test, since https://github.com/microsoft/go-winio/pull/185 just landed, I'll pull in the changes I have over in https://github.com/TBBle/hcsshim/commits/hack-base-layer-manipulation-with-branched-go-winio (i.e. https://github.com/TBBle/hcsshim/commit/c601b97e1376937c53e5416780e9824fc58fca4a but rebased and probably flattened in since it changes the LayerReader API).

I've pushed the 128Layers tests off to https://github.com/TBBle/hcsshim/commits/hack-base-layer-manipulation-with-128layers-test for now.

Oops. Build failed, because a change in go-winio API (syscall.Filetime has turned into x/sys/windows.Filetime) needs to be handled in containerd. I believe I've removed that code in latest containerd (it was forked from go-winio anyway), so I'll try bumping the vendoring.

Okay, that passed. However, the PR got super noisy from the vendor/ tree. It's 66k lines fewer overall, but that's still noisy.
It might make sense to land #942 first, and also bump containerd/containerd to v1.5.0-beta1, and perhaps go-winio to https://github.com/microsoft/go-winio/pull/185 or later separately? Neither vendor bump had any consequential changes as far as the compiler was concerned (except go-winio against containerd, as above), so they would be simple "edit go.mod; go mod tidy; go mod vendor" PRs to create.
Alternatively, is it time to drop the vendor/ tree? What's holding us back? Do we have back-compat concerns?

I think it'd probably be preferred to make vendor bumps in their own PRs

Sounds good. Now that #942 has landed, I'll create a PR for the containerd 1.5.0-beta1 bump, and then one to pull https://github.com/microsoft/go-winio/pull/185, and this can be rebased upon those.
It's actually only the top commit (811c3fc1fb75ea3e2ecaf2e3e3b26e3d71ebe964 to generate hard-links in the tarstream from hard-linked files on disk) which needs the vendor bumps, so if there any chance the containerd or go-winio bumps are going to be contentious or concerning, I can put that commit aside separately, and deliver a version of this either without hard-links, or with the earlier version of the hard-link detector that cached the ID of every file seen, just in case it turned out to have hard-links.

I've rebased on the containerd vendor update, opened #947 for the go-winio vendor update, and addressed all the open comments except the directory-modification-times question.

I've rebased on #947 so this should be in its final form now. Still 25% noise by file-count, due to test/vendor/Microsoft/hcsshim containing a copy of many of the same changes (by definition) but GitHub happily hides those by default. Basically, each commit includes go mod vendor in the test/ directory, to ensure it stays up-to-date and avoids big-bang fixes like #942 in the future.

So I tested by putting the "minimal" test-data hive from gabriel-samfira/go-hivex (originally from libhivex) in place of the empty file when creating the placeholder registry hives (i.e. borrowed and called readMinimalHiveContents from buildpacks/imgutil and the 128Layers test passed on my Windows 10 20H2 desktop.
So that's the fix, but I'm not sure how-best to actually pull that in here. I've updated https://github.com/TBBle/hcsshim/tree/hack-base-layer-manipulation-with-128layers-test, and the change is visible at https://github.com/TBBle/hcsshim/commit/446a977dc488c0002f0536e22223d6ea72b14842#diff-94af4df272f2d4cf11cff84cc70fe9ac3a715b0e2c13440bb5e131f56660806c, but I feel it's a little hacky to be relying on some other package's test data for this to work.
The origin story for 'minimal' suggests that there's no better source than this blob.
Also, because we need to pull data out of gabriel-samfira/go-hivex, it ends up trying to compile go-hivex, which reintroduces a CGO dependency. Because we never import it, it doesn't fail due to lack of the hivex library, but it is super-annoying after so long spent free of that.
So it would be better to hook this up as a go:generate step to memoise the minimal test data than rely on fetching it at run-time, which is like what buildpacks/imgutil is doing with this code anyway.
Because of the above, I'd rather leave this for a follow-up PR, since this PR is basically ready, and it's possible that the dependency of importLayer on valid empty hives is actually unintentional, or means the API is being misused in some way.

Awesome progress @TBBle! I figured I'd just +1 the value of having a stable, valid, minimal base layer in hcsshim. All your buildpacks/imgutil findings were right on - for us to find a minimal, distributable hive source for our non-runnable, but WCOW-daemon-pullable images was quite challenging and manipulating it though go-hivex until it was accepted by the various daemons, even more so. It would be awesome to have a supported, stable source.
As you said, it would be ideal to not have HCS (or whichever internal components) depend on those valid empty hives and base layer files, though perhaps beyond the scope of hcsshim. But our buildpacks/* components have no other need for the hives/empty files so we would gladly deprecate them if WCOW daemons no longer required them.

I rebased to master, but CI failed and and I need to rebase again once #959 merges.
I redid the "use hives instead of empty files" commit on https://github.com/TBBle/hcsshim/tree/hack-base-layer-manipulation-with-128layers-test to use go generate to avoid pulling CGO as a dependency or having a runtime dependency on external Go packages. See https://github.com/TBBle/hcsshim/commit/00ecf5c255335121d6a5f8463fcab7c67621767d for the implementation, replacing https://github.com/TBBle/hcsshim/commit/446a977dc488c0002f0536e22223d6ea72b14842#diff-94af4df272f2d4cf11cff84cc70fe9ac3a715b0e2c13440bb5e131f56660806c. Once this change merges, that commit will be the next PR.

Brief status update, after iterating on https://github.com/containerd/containerd/pull/4419 a little, it seems that the remaining problems I'm seeing there are now only when the tests are run in parallel, which is probably why my hacked-together "128 layers" test did not reproduce the same failures, nor could I reproduce them in isolation on my own machine.
So there may be still some related issues in HCS or hcsshim (i.e. I suspect that perhaps under load from multiple callers, DeactivateLayer or UnprepareLayer are returning before the resources are fully released, and causing subsequent ActivateLayer to fail with "file in use"), but this PR seems fine.

@ambarve could you take another look at this? I don't see anything wrong but I'm also not very familiar with this code :)

No update from my end, as far as I know it's ready-to-go, but maybe needs to be rebased? Once this lands, I have one more commit to PR for post-Server 2019.
Then I can start pushing to get the containerd-side merged, although I still need to create a hcsshim reproduction for the file-in-use problem around DeactivateLayer (which I reckon is unrelated to this change, but want to understand fully before I finalise the containerd work)

@katiewasnothere Can someone on your team provide insight into container.dll/vmcompute constraints around base image creation/consumption?

@TBBle I know this has been open for a long time but we are looking at some legal aspects around exporting base layers (i.e layers of official windows container images). I will post a comment here if we need to make any changes and then we can merge this. Thanks for your patience.

I had wondered if any legal concerns were going to appear. I figured they shouldn't affect this code, because I'm not really introducing anything you can't already do just by grabbing the tarball for the base layer from MCR directly; that was in-fact my initial acceptance test for base layer export. I also could do basically the same thing with docker pull and then tar czf since a base layer is pretty-much a tarball of the extracted directory anyway.

@TBBle I think the export gzip option is bugged, creating a tarball with this option then attempting to extract it results in error.
[...]
x Files/Windows/System32/wbem/ndistrace.mof
x Files/Windows/System32/wbem/ntevt.dll
x Files/Windows/System32/wbem/ntevt.mof
x Files/Windows/System32/wbem/tspkg.mof
x Files/Windows/System32/winevt
x Files/Windows/System32/winevt/Logs
x Files/Windows/System32/winevt/TraceFormat
x Files/Windows/SystemResources
x Files/Windows/SystemResources/crypt32.dll.mun
x Files/Windows/SystemResources/propsys.dll.mun: truncated gzip input
tar: Error exit delayed from previous errors.

Yeah, I hit that too, as I recall, but never looked into it. However, that's not part of this change, AFAIK, it was already like that, and I didn't make any chances to the gzip output implementation.

@TBBle Ah, understood. Thanks for being patient with my side quests.

@ambarve and @TBBle what is the status on this?

As far as I know, https://github.com/microsoft/hcsshim/pull/901#issuecomment-859764981 is still the current state. I haven't touched this in a while, so it might need rebasing.

@ambarve did you finish checking the legal aspects?

@ambarve @katiewasnothere any updates regarding https://github.com/microsoft/hcsshim/pull/901#issuecomment-859764981?

I will post a comment here if we need to make any changes and then we can merge this. Thanks for your patience.

@ambarve It looks like that @TBBle is very patient, he's waiting for half a year already. Maybe it's time to finally admit that legal issue is not in fact an issue because layer exporting does not redistribute anything to anyone and move on with this PR that currently blocks BuildKit implementation for Windows containers?

First of all, apologies for the delay in responses on this thread. We had reservations about the legal aspects of this PR for two reasons. First the restrictions around creating (& publishing) empty scratch layers that are not bootable container layers. Most probably this won't be an issue but we are working with our internal teams to get a confirmation on it. I will try to provide an update here ASAP (hopefully within a week). Second, for the base layer export we still need to make sure that the exported base layer is correctly marked as a foreign layer (Non distributable layer) so that it won't be pushed to a repository. @TBBle  can you please confirm if the base layers that are exported will be marked as foreign layers? Can you also please rebase this PR? Otherwise, this PR looks good and ready to be merged as soon as we have answers for both of these questions.

There's nothing in what I have here that would mark an exported base layer as foreign, because the intended use-case is not for exporting layers that were imported from elsewhere, but for exporting layers built on 'scratch', which would not be marked foreign by default.
In fact, while you can re-export an imported base layer, you cannot export an imported non-base layer, i.e. if I pull a Windows Server image of any kind, I get the base layer plus a layer for the updates, but due to the on-disk storage, I can only export that first base layer. (This is not something I plan to solve, as it's intrinsic to HCS at the moment, and so tooling here can't really do anything about it. This only affects the HCS-driven layer export process, a generic diff of course can still re-export imported layers without Windows-specific metadata.)
hcsshim layer-handling has no concept of "foreign layer" at this time, that would have to be handled by (or in concert with) a caller like containerd; I'm not sure if containerd carries the 'foreign layer' concept down into the layer storage (Snapshotter) system, since that's an attribute of the layer reference in the manifest, not intrinsic to the layer itself, as we recently discovered in a BuildKit discussion on the same topic: See https://github.com/moby/buildkit/pull/2561 and https://github.com/moby/buildkit/issues/2555.
I haven't checked containerd, so I don't know if the Content Store even maintains the content-type of an object, and how it would handle seeing the same object (by hash) with different content types in different contexts. That BuildKit PR touches upon stargz-based layers which actually have a similar problem.
As an idea, a newly-created empty layer could have a marker that indicates it's safe for push, but it would still be on higher-level tools to recognise this annotation (it could be exposed by hcsshim import API) and apply the appropriate foreign content type when creating a manifest that references the layer.
Of course, all of this is only if one uses the hcsshim code and/or honours the resulting flags. There's no way to prevent someone exporting a layer into a manifest, editing the content type or ignoring some flag, and pushing it to a registry. After all, if one wanted to mark a layer as foreign that isn't currently (i.e. I built it myself, and don't want it repushed), it's the same process.
Technically, there's no requirement in OCI that foreign and non-foreign layers are actually interchangeable; so a marker could be included inside the tar stream to carry a "from-scratch" layer flag.
However, all discussion of flag files on-disk or in tar streams is complicated by the existence of both base and non-base layers without that flag, created by existing tools in the ecosystem.

Hmm. Rebase built and passed, but I guess we still aren't running the actual tests on CI, and I don't have access to a Windows Server 2022 machine to verify if https://github.com/TBBle/hcsshim/commit/1803a218685ea1c449e1383639dbb459b1473fc7 is needed.
I'm not even sure if the current tests would trigger that case, I'll try and put together a test-case that does trigger it. I've relied on the containerd snapshotter test suite to test it before now, but that work needs a bunch more rebasing (and I know it has conflicts) so it makes more sense to add the tests here.

Gaaah. So I finally think I found the reason I was having problems on RS5 in the containerd Snapshotter test suite, and it was me all along, leaking a reference to an opened file: https://github.com/microsoft/hcsshim/compare/0ce0a83127aa7ac90eae33b7b835781b70233333..3577daea1d5860867edce38a0129f8cd3cfafe45
Looks like somewhere after RS5 (and before 19042), this leaked reference stopped preventing deletion of the directory, which is why the error never reproduced for me locally. And any tests with the wclayer CLI binary closed the file when the owning process closed, so that never reproduced either. I'm actually surprised that the containerd test suite ever worked, so perhaps the leaked reference was only a problem temporarily.
I also now have some integration tests for the base layer creation changes here which I'll pull into this PR soon.
However, I suspect based on https://github.com/microsoft/hcsshim/pull/901#issuecomment-1068407627 that I'll need to split this PR into two, one for non-bootable base layer creation, where it sounds like the concern was minor:

First the restrictions around creating (& publishing) empty scratch layers that are not bootable container layers. Most probably this won't be an issue but we are working with our internal teams to get a confirmation on it. I will try to provide an update here ASAP (hopefully within a week).

and one for base layer export where the concern was legal. In this latter case, the worst-case is that there's an existing ecosystem-wide legal issue that my PR has surfaced:

Second, for the base layer export we still need to make sure that the exported base layer is correctly marked as a foreign layer (Non distributable layer) so that it won't be pushed to a repository.

My non-reassuring reply was focussed on this part.
For purposes of BuildKit functionality and (IIRC) the containerd snapshotter test suite, we don't actually need to export base layers, just be able to create them. And I reckon I could just punt 8778526ecb1d2a3df2c590bef0956b887159630e to bring us into that state, so this wouldn't be hard to do, if it's enough to get the rest of this PR merged.

Latest rebase pushes the changes I've broken out into #1374 to the front, so that (assuming that lands soon) this PR is reduced to just those things stuck behind the legal concerns.

Hey folks, jumping in here to provide an update and clear up some confusion from the Microsoft side. What held this up for so long were some reservations from our legal team on enabling an open-source tool to easily package and redistribute Windows binaries. With the secure supply chain executive order, and our general company policies on security, we are moving very cautiously on topics like these. With that said, I'm here to bring the good news that these issues have resolved, and this PR is good to move forward.
I know it's been incredibly frustrating for the community here. I want to emphasize that we are here to support the community and do the best we can to make things easy for you. This definitely took longer than it should have and we're changing how things are done on our end to ensure it doesn't happen again. If you have concerns or questions, always feel welcome to reach out to me. (Brandon Smith (MS) on K8s Slack)
@ambarve would you be able to move this forward from here?

Glad to hear that. I've clearly got some rebasing and updating to do, I'll try and make some time for that soon but I'm pretty snowed under at the moment. Maybe on the coming weekend I can flush the rebasing through, at least.
Due to https://github.com/microsoft/hcsshim/issues/1375+https://github.com/containerd/containerd/issues/6896, I can't easily test this with the containerd Snapshotter test-suite as I did in the past. So I'll also have to pull-in and complete the tests I mentioned in April, and ensure everything's working on both LTSC 2019 and LTSC 2022 (although I'm confident it will be, since I squashed one bug and know of only one other likely stumbling point, for which I have a proposed fix). At least those platforms are available on GHA now.

@TBBle I'm trying to resolve #1375 with this https://github.com/microsoft/hcsshim/pull/1463 if you want to take a look. Still going back and forth on that

Sorry, I haven't had a chance to look closely at #1463 yet. It shouldn't block this (unless an API I need changes...) as I think it's better to do tests in this repo anyway. I've rebased my existing work (it wasn't as bad as I feared, most of the big scary conflict warnings turned out to be delete/edit over test/vendor, and they're trivial to resolve.
I've pushed my WIP tests over to https://github.com/TBBle/hcsshim/tree/layer-manipulation-tests and will perhaps go over them tonight or tomorrow, and try and get them clean-enough (and passing-enough, and high-coverage-enough) to pull into this PR, in which case I'd be confident this code is ready-to-go.
I see we now have a containerd integration test in the CI suite, but my Go practice is a little rusty, and I'm not sure if it will pick up the checked-out hcsshim in its containerd build as I suspect it will prefer the vendored version. So once #1375 has been resolved in some way, if we force teach the containerd integration test to build against the checked-out hcsshim, then we get the containerd snapshotter test suite also running for us, once that side lands too in https://github.com/containerd/containerd/pull/4419 or similar. (There's significant runway before that PR lands, which is another good reason to land this code here with its own tests.)

Sorry, I haven't had a chance to look closely at https://github.com/microsoft/hcsshim/pull/1463 yet.

No worries at all. I found out I'm calling that FormatWritableLayerVhd function internally in a helper in the same package also which is rather annoying.. Hoping to have a clearer picture this coming week on what we wanna do there.

I see we now have a containerd integration test in the CI suite, but my Go practice is a little rusty, and I'm not sure if it will pick up the checked-out hcsshim in its containerd build as I suspect it will prefer the vendored version.

The containerd integration suite here should run against/build a shim from whatever is in the current PR. So containerd main + your branch for hcsshim.

The problem for the integration test is that the code I'm working on isn't in the shim, it's an in-tree snapshotter in containerd linking against hcsshim directly. If it was an external snapshotter instead, that'd be a different question (and also solves the manifesting blocker in-passing). Although there isn't currently support as I recall an external mounter in containerd, so we'd be a significant refactoring away from being able to decouple fully anyway, if that was a useful direction.
On option would be to programmatically add a replace rule to containerd's go.mod to use the local checkout of hcsshim for its in-tree work. That (but manually) is how my testing of the snapshotter worked in the containerd tree. That whole idea though is somewhat flawed, as unlike the runtime shim, there isn't a stable API/ABI boundary for this part, so such tests would fail when hcsshim evolves its API in breaking ways, ala 0.9 and (I assume) 0.10.

@TBBle Ahh gotcha gotcha, you meant the containerd suite. Yea it will still use whatever is in its go.mod for code it uses hcsshim as a library for. External snapshotter solving the manifesting issue is an interesting thought also.. That's actually one of the suggestions we heard internally, but I'm not sure it makes sense for the windows snapshotter to not be built in

I also had "take the snapshotter external" as an option in the first "hcsshim ABI broke hard" ticket; in a green-field development, knowing what I know now, I'd have built the Windows snapshotter as an external snapshotter in this repo, since it parallels the external runtime shim and better-isolates containerd from vagaries of HCS/computerstorage evolution. (And this is the direction the OS X containerd work was strongly encouraged by the containerd team, which is what put it on my radar at all; in that case, they also farmed off mounting by defining a CLI utility as the API boundary).
But given the half-decade-or-so weight of code for the Windows Snapshotter already in containerd, as well as having access to the snapshotter test suite once we get that working, it'd be a time-costly and burden-shifting change that'd need more justification and stakeholder buy-in than I'm currently aware of.

Carrying back results from the now-unblocked testing with my containerd PR that depends on this work, and it fails in what looks like https://github.com/microsoft/hcsshim/pull/901#issuecomment-791880325 even on Windows Server 2019. So either something else has gone wrong, or there's been a platform update that pulled in whatever change introduced that in later Windows builds.
So for this PR, I'll need to finish my existing test suite work, and get it to the point where I can replicate that issue locally, and then rebase/reinstate/recreate https://github.com/TBBle/hcsshim/commit/1803a218685ea1c449e1383639dbb459b1473fc7 into this PR as well, since it looks like it's all-versions necessary now.

Converted to draft as I'm currently moving countries, and hence don't have time or computing power for at least the next month (and maybe longer... we'll see) to rebase and resolve merge conflicts, let-alone complete the work described in https://github.com/microsoft/hcsshim/pull/901#issuecomment-1211456887.

So for this PR, I'll need to finish my existing test suite work, and get it to the point where I can replicate that issue in this repo directly, and then rebase/reinstate/recreate TBBle@1803a21 into this PR as well, since it looks like it's all-versions necessary now. (A quick test suggests the cherry-pick will be easy, it didn't conflict anywhere except go.mod/go.sum and vendoring)

To generate the minimal hive, how about something like this:
package main

import (
	"fmt"
	"log"
	"os"
	"syscall"

	winio "github.com/Microsoft/go-winio"
	"golang.org/x/sys/windows/registry"
)

//go:generate go run golang.org/x/sys/windows/mkwinsyscall -output zsyscall_windows.go main.go
//sys   RegSaveKeyW(key syscall.Handle, file *uint16, access uint32) (regerrno error) = advapi32.RegSaveKeyW

func main() {
	privileges := []string{"SeBackupPrivilege"}
	if err := winio.EnableProcessPrivileges(privileges); err != nil {
		log.Fatal(err)
	}
	defer winio.DisableProcessPrivileges(privileges)

	k, existing, err := registry.CreateKey(syscall.HKEY_CURRENT_USER, "tempKey", registry.ALL_ACCESS)
	if err != nil {
		log.Fatal(err)
	}
	defer registry.DeleteKey(syscall.HKEY_CURRENT_USER, "tempKey")

	fmt.Println(k, existing)

	saveTo := "C:\\Users\\Administrator\\reg\\save.dat"
	if _, err := os.Stat(saveTo); err == nil {
		os.Remove(saveTo)
	}

	if err := RegSaveKeyW(syscall.Handle(k), syscall.StringToUTF16Ptr(saveTo), 0); err != nil {
		log.Fatal(err)
	}
}

This exports an empty key. That exported file should be usable as a valid hive. The tempKey becomes the root of the new hive.

Use the offline registry library functions instead https://learn.microsoft.com/windows/win32/devnotes/offline-registry-library-functions

@TBBle
Created a PR against your branch to generate empty hives using the offline registry library. I know you don't have time now, so have a look at your earliest convenience.

#1637, the successor for this PR, has merged.


Data Plane: Generate log information in a standardized manner

Discussed in https://github.com/fybrik/fybrik/discussions/890
Create a single log structure to be used by both the control plane and the data plane components as is described in the visibility high level design document.

[ ] Change the arrow flight service to generate log entries according to the new structure
[ ] Change the data movement operator to generate log entries according to the new structure

Dependent on #926

@simanadler, @cdoron, can we close it?


Can't edit Geometry Transform in Properties

When Geometry is selected, there is no Transform property displayed in the Properties Window.
4.0 GUI

3.3 GUI

I would like access to the Geometry Transform.

There's no standalone Transform that lives with the Geometry, instead the Geometry is attached to a Frame (along with potentially other objects). Possible solutions:

Show Frames in navigator and they are edited independently (adds clutter but maybe useful anyway)
Add Frame properties to Geometry properties (may have side effects if the frame has been re-used)
Open to other ideas/suggestions

Ideally we could select "Frame" (from the known Socket)  to edit the Frame it is on and also have a "new" option to create a new Frame (in which case we create an OffsetFrame) that would allow you to enter any transformation you desire relative to any Body or Ground you desire.


#171

#171 MakeValidFieldName should prevent names that are reserved keywords bug

Awesome thanks for fixing the warnings and being open to feedback. :rocket:


Feature Request: Add support for new iOS 13 bluetooth permission/authorization feature

Current Behavior

The library properly emits an "unauthorized" state when the user is not allowed to use bluetooth.

However there is no way to determine through the library what the reason for this unauthorized state is. In iOS 13 the user can explicitly deny or approve bluetooth permission, since a dialog will be displayed by default for all new, updated and existing apps.

New behavior
There should be a way to check the authorization status (via CBManagerAuthorization) when bluetooth manager reports the "unauthorized" state to determine of a user denied the permission or not (and allow us to act accordingly in the UI).

I also experienced the same issue on iOS 13

I also experienced the same issue on iOS 13

hi @bitcrumb, I found out that BleManager.onStateChange can emit "Unauthorized" status

@ntrannghia Yes, that's what I explained in my issue. I rephrased a bit to make this more clear. What I am talking about however, is that there is no way to retrieve the reason for this "unauthorized" state. This is communicated through a a new API availablein iOS 13. The CBManagerAuthorization enum. This part has not been incorporated yet.
Also, the current version of ble-plx does not compile on the beta Xcode 11.

@bitcrumb Yes, I notice the same.

node_modules/react-native-ble-plx/ios/RxBluetoothKit/RxCBPeripheral.swift:364:121: 'canSendWriteWithoutResponse' is only available in iOS 11.0 or newer

There's a PR opened that presumably fixes this: https://github.com/Polidea/react-native-ble-plx/pull/527

Yes idd, I noticed that the PR has merged. Thanks for that :)
I think the only thing that remains now is indeed to have access to the enum containing the reason for being unauthorised to use bluetooth. This can come in handy to display certain specific UI/feedback to the user.


”Grajduri”, ”Cărbunari”, ”Corcodel”, ”Lunca” are listed in the drop-down menu

Description:
When user writes ”Grajduri”, ”Cărbunari”, ”Corcodel”, ”Lunca” the locations appear in the drop-down menu next to the comune and the county which is a part according to the law.
Precondition:
The website is up an running.

Step 1
Write in the search bar "Grajduri"

Expected results
The location appears in the drop-down menu as "Sat Grajduri, IAȘI (Grajduri)".

Step 2
Press "x" button.

Expected results
The location was deleted from the search bar.

Step 3
Write in the search bar "Cărbunari"

Expected results
The location appears in the drop-down menu as "Sat Cărbunari, IAȘI (Grajduri)".

Step 4
Press "x" button.

Expected results
The location was deleted from the search bar.

Step 5
Write in the search bar "Corcodel"

Expected results
The location appears in the drop-down menu as "Sat Corcodel, IAȘI (Grajduri)".

Step 6
Press "x" button.

Expected results
The location was deleted from the search bar.

Step 7
Write in the search bar "Lunca"

Expected results
The location appears in the drop-down menu as "Sat Lunca, IAȘI (Grajduri)".


Inconsistent component directory structure

Current behavior

Expandables put all the less code in https://github.com/cfpb/capital-framework/blob/canary/src/cf-expandables/src/cf-expandables.less, on the other hand, pagination imports the less code via /molecules/ directory https://github.com/cfpb/capital-framework/blob/canary/src/cf-pagination/src/cf-pagination.less#L35

Expected behavior

The less should be separated into a /atoms/, /molecules/, /organisms/, directories OR included in the main cf- less file for the component.

Proposed structure:
Remove the subdirectories and prefix the less files with the atomic component they contain.
Consider the current structures:
cf-forms
atoms/
cf-forms.less
molecules/
organisms/

cf-expandables:
cf-expandables.less

For the first example, the problem is it is not immediately apparent what and how many atomic components are in the package, without snooping into the sub-directories. Also, the atomic component files do not have the same name as the class name they contain, such as atoms/label.less, which contains a-label.
For the second example, the problem is the same, but for different reasons. To check what and how many components are in the package, the cf-expandables.less needs to be opened the browsed for a-, m-, and o- prefixes.
Instead, these directories could look like:
a-label.less
a-legend.less
a-multiselect.less
a-select.less
a-text-input.less
cf-forms.less
m-btn-inside-input.less
m-form-field.less
o-form.less

And
cf-expandables.less
o-expandable.less

I like it. I like having all the files in the same folder for ease of maintenance and to better understand what elements are associated with each component.

Sounds good to me. The prefixes also keep them organized and we lucked out that our system of prefixes happen to be in alpha order. I say lets do this whenever someone has a chance.


Fix ansible.builtin twice

Closes #128

@aalaesar unfortunatelly there was a bug introduced in latest version. This is a hotfix.


Add example notebook with JUMP UMAP processing

Description
This PR adds an example notebook to help demonstrate coSMicQC use with JUMP by performing a UMAP analysis. The output generally show where outliers occur within the embeddings from a sample of BR00117012 sourced from the cpg0000-jump-pilot JUMP dataset.
Additional notes:

I took a moment to update the cosmicqc_in_a_nutshell notebook which recently was reorganized / was lacking display of outlined cells within the CytoDataFrame.
As a result of this modification I also noticed some unnecessary print statements which I removed.
The coverage was changed I believe due to these print statements being removed.
I've double checked locally and saw that the interactive Bokeh plots render properly through Sphinx (which means they should render within GitHub Pages).

Thanks for any feedback you may have!
Closes #23
Closes #83
Closes #90
What kind of change(s) are included?

[x] Documentation (changes docs or other related content)
[x] Bug fix (fixes an issue).
[ ] Enhancement (adds functionality).
[ ] Breaking change (these changes would cause existing functionality to not work as expected).

Checklist
Please ensure that all boxes are checked before indicating that this pull request is ready for review.

[x] I have read and followed the CONTRIBUTING.md guidelines.
[x] I have searched for existing content to ensure this is not a duplicate.
[x] I have performed a self-review of these additions (including spelling, grammar, and related).
[x] These changes pass all pre-commit checks.
[x] I have added comments to my code to help provide understanding
[ ] I have added a test which covers the code changes found within this PR
[x] I have deleted all non-relevant text in this pull request template.

Thanks @gwaybio and @jenna-tomkinson for your earlier reviews! I've added a bunch of changes based on what you mentioned and this could use another look when there's a moment.

Thanks so much @jenna-tomkinson and @gwaybio for your reviews! I've addressed all comments and added new figures based on the results. I'll now merge this in.


Inconsistencies in the Txn list.

Two issues:

All tips are shown as negative amounts, same with supports. Whereas these should be positive, or negative depending on where is it accessed from(-ve for sender, +ve for receiver).

All transactions which are basically dust(fee only txns) are also shown in the UI.

What's causing them

Not all amount can be assumed to be negative
https://github.com/lbryio/lbry-app/blob/master/ui/js/selectors/wallet.js#L58

It should be tx.value instead of tx.amount
https://github.com/lbryio/lbry-app/blob/master/ui/js/selectors/wallet.js#L24

Disclaimer: Preliminary diagnosis, conducted at 3 AM, subject to market risks.

Thanks @hackrush01 . Can confirm 1 needs to be fixed.
For #2, the only type of TX that I can think (which would cause a problem at the moment) where tx fee = amount (or at least how its shown now) is a claim abandon. I think we need to add a new category for Abandoned Claims. The amount should show the LBC amount going back into the wallet, along with the publish fee as the fee.

First one is fixed in latest RC, 2nd one still needs to be looked at. I'll open a separate ticket for the abandon scenario.

@hackrush01 see https://github.com/lbryio/lbry-app/blob/master/ui/js/selectors/wallet.js#L24
They're supposed to be filtered there.


Add support for HTML5 HTML Helpers in ASP.NET MVC

Feedback from UserVoice site: https://aspnet.uservoice.com/forums/41201-asp-net-mvc/suggestions/6599921-add-support-for-html5-html-helpers-in-asp-net-mvc
Copying the contents below.
t would be great if you had the ability to use HTML5 together with the built in HTML Helpers in ASP.NET MVC. Being able to use code such as:
@Html.TextBoxFor(model => model.EmailAddress, InputType.Email, "Placeholder goes here")
or:
@Html.Html5Range(1, 100 , 0, 23, null)
There is currently an external open source project maintained on Codeplex (http://mvchtml5.codeplex.com/) that does this, however it would be great to have this functionality available in Visual Studio by default.
Being able to support HTML5 elements such as input types and video/audio elements would be great.

Color
Email
Search
Time
Tel
File
Range & Number
Audio & Video

yes it could be nice if  it is done.

BTW Html.EditorFor() supports the HTML5 input types.

Do you know about HtmlTags?
HtmlTags is composable through a fluent interface. The ASP.NET MVC team could still support the current set of extension methods in addition to allowing MVC developers to build their own more easily.
I would be weary about trying to support specific input types (because there is always going to be more), but instead make it easier to build any input type more easily.

We will take another look at HTML5 improvements in helpers in the next release.

These kinds of things work super will with the <input .. /> tag helper.
I don't believe we have any plans to add features to HTML helpers. Suggest closing /cc @DamianEdwards @Eilon

Agreed, I'd recommend ppl use Tag Helpers to get the best experience here.


The pop items could have a bigger list

because of the all of the items in the new update, dunno just an idea,
i know i can setup a higher price at the items but would be useful

You can have as many items as you want in the Popup Items. Press Enter to add new items and Delete to remove them.


[stable/nats] Fixes default value of auth.user documented in README.md

Is this a new chart

NOTE: We're experiencing a high volume of PRs to this repo and reviews will be delayed. Please host your own chart repository and submit your repository to the Helm Hub instead of this repo to make them discoverable to the community. Here is how to submit new chart repositories to the Helm Hub.

What this PR does / why we need it:
Which issue this PR fixes

Fixes default value of auth.user documented in README.md

Special notes for your reviewer:
Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

[ ] DCO signed
[ ] Chart Version bumped
[ ] Variables are documented in the README.md
[ ] Title of the PR starts with chart name (e.g. [stable/chart])

@vpartington
Please bump the chart version to latest. Thank you.

It's a minor change in the README.md. Does that also require the chart version to be bumped?

@vpartington
yes please.

Hi @vpartington
As @zanhsieh explained, every change (even if it's a minor correction in the README.md) requires bumping the version in the Chart.yaml
In your case, you need to bump the patch version.

@zanhsieh @juan131 Thanks. I have bumped the patch version number.
I have amended my earlier commit and force pushed to the branch so that the change is in a single commit.

/retest


⚠ ClamAV has degraded performance

In d402828, ClamAV (https://spamassassin.apache.org/) experienced degraded performance:

HTTP code: 200
Response time: 195 ms

Resolved: ClamAV performance has improved in 310993a.


add support for custom offsets

for those struggling to get auto offset detection to work

I dont know. If someone struggels to get auto offset work, isn't that a hint there is something wrong that will not be fixed by adding offsets?

I dont know. If someone struggels to get auto offset work, isn't that a hint there is something wrong that will not be fixed by adding offsets?

Your point is well taken, do what you will. Had a couple saying they're stuck in 0.3 because botty 0.4 won't get past the offset detect log and they showed me screenshots. In my laziness I figured this fix would be easier than having to do video chat or something for everyone with the issue.

I added some debug messages about the maximum score if the templates were not found and lowered it a bit. Also added a warning if your scores are lower than usual.
I am sticking to my opinion that fixing the symptom will not fix the issue for them. You will just here follow up issues like. Stuck at eld waypoint etc.

I can see where you're coming from. I don't know if I would have tried this hadn't we done something similar just this week, however. Would you agree with this same reasoning for #206 ? Either there's inconsistency with template matching (maybe platform based?) or the users aren't setting their configurations right, but either way a symptom was addressed at the cost of a feature.


MauroDataMapperPlugins - Add a PDF Exporter

Placeholder issue for sprint management. Real issue is found here:
https://github.com/MauroDataMapper-Plugins/mdm-plugin-explorer/issues/37
* MauroDataMapper-Plugins is a different github organisation so the issues within cannot be linked to the mdm-state project or any milestones

Associated task has now been completed.


V0.2 docs process mode

Description
Update maro process mode docs
Linked issue(s)/Pull request(s)

Type of Change

[ ] Non-breaking bug fix
[ ] Breaking bug fix
[ ] New feature
[ ] Test
[X] Doc update
[ ] Docker update

Related Component

[ ] Simulation toolkit
[ ] RL toolkit
[ ] Distributed toolkit

Has Been Tested

OS:

[ ] Windows
[X] Mac OS
[ ] Linux

Python version:

[X] 3.6
[ ] 3.7

Key information snapshot(s):

Needs Follow Up Actions

[ ] New release package
[ ] New docker image

Checklist

[ ] Add/update the related comments
[ ] Add/update the related tests
[ ] Add/update the related documentations
[ ] Update the dependent downstream modules usage

Codecov Report

Merging #230 (f385e5e) into v0.2 (5ff0895) will decrease coverage by 2.49%.
The diff coverage is 37.20%.

@@            Coverage Diff             @@
##             v0.2     #230      +/-   ##
==========================================
- Coverage   68.22%   65.73%   -2.50%
==========================================
  Files         103      110       +7
  Lines        4806     5221     +415
==========================================
+ Hits         3279     3432     +153
- Misses       1527     1789     +262

Flag
Coverage Δ

unittests
65.73% <37.20%> (-2.50%)
:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

...o/simulator/scenarios/citi_bike/business_engine.py
74.21% <ø> (+1.17%)
:arrow_up:

...mulator/scenarios/vm_scheduling/business_engine.py
20.40% <20.40%> (ø)

...mulator/scenarios/vm_scheduling/virtual_machine.py
35.71% <35.71%> (ø)

...simulator/scenarios/vm_scheduling/frame_builder.py
37.50% <37.50%> (ø)

...ulator/scenarios/vm_scheduling/physical_machine.py
43.47% <43.47%> (ø)

maro/simulator/scenarios/vm_scheduling/common.py
57.89% <57.89%> (ø)

...ro/simulator/scenarios/vm_scheduling/cpu_reader.py
97.61% <97.61%> (ø)

maro/simulator/core.py
80.00% <100.00%> (ø)

maro/simulator/scenarios/cim/business_engine.py
85.60% <100.00%> (ø)

maro/simulator/scenarios/vm_scheduling/events.py
100.00% <100.00%> (ø)

... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ff0895...f385e5e. Read the comment docs.


react/no-typos emit false error.

This rule fails only on external proptypes declaration. Using static class field propTypes works fine.
// validators.js
import PropTypes from 'prop-types';

export const validator = PropTypes.shape({ ... });

// Component.jsx
import { validator } from 'validators';
function Component({ value }) { ... }
Component.propTypes = {
  value: validator.isRequired,
//                ^-----------^   - Typo in declared prop type: isRequired (react/no-typos)
};

hmm, that should definitely work.

I get errors on ImmutablePropTypes.map and ImmutablePropTypes.list
Typo in declared prop type: map  react/no-typos
Typo in declared prop type: list  react/no-typos

Please help. Currently it breaks my build, so I will mute this rule until you fix it

@JustFly1984 please file a separate issue for that.

This is really bothersome when you have lots and lots of stateless components.

I've looked into this, but I'm unable to reproduce with the following test case (which produces no errors):
import { validator } from 'validators';
function MyComponent(props) { return (<div>{props.myProp}</div>) }
MyComponent.propTypes = {
  myProp: validator.isRequired
};


Improvement Improve readability for runtime filters in profile string

Proposed changes
For example,  run sql below on TPCH 1g
SELECT   s_acctbal,   s_name,   p_partkey,   p_mfgr,   s_address,   s_phone,   s_comment FROM   part,   supplier,   partsupp WHERE   p_partkey = ps_partkey   AND s_suppkey = ps_suppkey   AND p_size = 15   AND p_type LIKE '%BRASS' ORDER BY   s_acctbal DESC,   s_name,   p_partkey LIMIT 100
Now runtime filter's profile is displayed as below

After this patch, runtime filter's profile has been changed to:
(effective in await time)

(not effective in await time)

Problem summary
Describe your changes.
Checklist(Required)

Does it affect the original behavior:

[ ] Yes
[ ] No
[ ] I don't know

Has unit tests been added:

[ ] Yes
[ ] No
[ ] No Need

Has document been added or modified:

[ ] Yes
[ ] No
[ ] No Need

Does it need to update dependencies:

[ ] Yes
[ ] No

Are there any changes that cannot be rolled back:

[ ] Yes (If Yes, please explain WHY)
[ ] No

Further comments
If this is a relatively large or complex change, kick off the discussion at<EMAIL_ADDRESS>by explaining why you chose the solution you did and what alternatives you considered, etc...

LGTM

TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 36.59 seconds
load time: 506 seconds
storage size:<PHONE_NUMBER>6 Bytes
https://doris-community-test-1308700295.cos.ap-hongkong.myqcloud.com/tmp/20221110120016_clickbench_pr_43618.html


Explain how rebuild works with with docker build cache

Code of Conduct

[X] I have read and agree to the GitHub Docs project's Code of Conduct

What article on docs.github.com is affected?
https://docs.github.com/en/codespaces/developing-in-a-codespace/rebuilding-the-container-in-a-codespace
What part(s) of the article would you like to see updated?
Docker has a nifty little feature called cache mounts.
The current explanation says that it will reuse build layers while using custom Dockerfile. However no explanation is provided on how cache mounts work if at all, their constrains and such when triggering a normal rebuild.
Additional information
No response

@sansmoraxz Thanks for opening this issue! I'll get this triaged for review ✨

@sansmoraxz - thanks for raising this issue. I feel that this is not a level of detail we need to go into in the "Rebuilding the container in a codespace" article. However, I'll run this by product management for a second opinion.

I am not interested in Email please

Just wanted to check in on this issue 💛 @hubwriter has product management been able to provide their opinion on this?

@nguyenalex836 - Thanks for the bump. I'll check again.

Thanks for raising this PR. However, I'm going to close this out as this isn't a level of detail we want to go into in this article.


[ADAM-1421] Add java API for loadReferenceFile.

Resolves #1421. Also fixes JavaADAMContextSuite to actually use JavaADAMContext instead of ADAMContext.

Coverage decreased (-0.09%) to 82.154% when pulling 64cafd36f8907aef3f584cfb7cd698e8221cf1ed on fnothaft:issues/1421-reference-file-java into 37b971a3afb80598ffecd5275aff6c3ceaa96ef3 on bigdatagenomics:master.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/2023/
Test PASSed.

Thank you, @fnothaft!


Agent encountering/responding with Error Code 500 while refreshing files associated with configuration file

From slack thread: https://positpbc.slack.com/archives/C05D7NZD52S/p1726068297152959
This might be something specific to my environment, but while editing files in Emacs, the publisher was constantly trying to refresh the project and sometimes produced errors like the following, which are temporary files used while in-flight edits are being performed. I received several of these errors:

Q> How can I get these refresh errors to stop? They are happening even while the publisher view is not open. Do I need to remove the extension?
Unfortunately, there is no specific way to disable them. We rely on them for a large amount of functionality, so a bug in this area is not very mitigable. Without uninstalling the extension, you could disable it (and then enable it back when you want to try its functionality). Otherwise, you could uninstall it until we resolve that issue.
Q> OK. I’ll disable for now. I didn’t expect it to be doing background things while not thinking about publishing.
Looking closer at the error code, it is your symlink .#bug.py that is giving the agent problems. This is a good bug and one which we hadn’t seen before. Thanks!
It is an interesting comment to consider: “I didn’t expect it to be doing background things while not thinking about publishing.“. As an extension, part of our functionality is loaded immediately by VSCode, ahead of our activation of our UX. The background functionality supports the command pallet commands and our file-specific entrypoint detection. We have taken a simple path to start with, but could certainly be much more thoughtful about what happens ahead of showing the UX.

May be a duplicate or fixed by #2323


resnet with COCO

hi~
I wonder if you have test faster-rcnn on COCO data with resnet101? I have trained it a few times, but it just get AP(0.5-0.95) = 0.063.
Thank you in advance!

sorry, I haven't


Question - how to get "offset" for an image?

Hi,
Want to add an image - let's say in column G. I don't want to add directly at the border, want to move it to the right.
This could be done with set_position2 (zcl_excel_drawing) and adding a col_offset. I thought this is the offset from the left border of colum G, but seems to be an offset from the first column left. So I need to have this "value" from A to G. It's not simply counting all the column widths together, it's a different number.
Have done some tests, result for me was that I have to add a col_offset of 19080 (created an ecxel in SAP, checekd the drawing1.xml, then moved my image to the right, checked again the drawing1.xml).
So my question - where to get this number from?
Thanks for your help
Michael

Cleanup of open issues:
This topic is older than one year:  Unless someone comments that this topic should be kept as an open issue I'll close this in about two weeks time.

Hello Michael,
the number is indeed the offset to the top/left border of the cell the drawing is attached to .
But it is given in emu and not in pixels.
See method zcl_excel_drawing=>emu2pixel to retrieve the number of pixels.
For you col_offset of 19080 at a (default) 96 dpi you'd get
19080 * 96 / 914400 2,00314961 ~ 2 pixel ( I tried the same with my excel and found the value to be 19050 which gets exactly to 2 pixel )


docker compose up -d calcom is misleading, it still runs database

I run my own postgresql for several other things, so I connected calcom to that as well. It works fine there.
docker compose up -d calcom says it just runs calcom however it runs database too
I have deleted the database config out of the docker file and it seems to work just fine. So this leads me to believe it should be doing that already?
I see the prisma thing is also marked there to be deleted.
I believe the docker compose up -d calcom should ONLY run calcom and nothing else. As it claims to do.

General consensus is separate db containers for each service: https://www.reddit.com/r/docker/comments/dwlvt6/one_database_per_container_or_one_for_everything/

Your docker gives the option to use a separate database, so the poor performance community isn't relevant.
If you're already using a remote db, there's no reason to run another database that isn't even being used.

General consensus is separate db containers for each service: https://www.reddit.com/r/docker/comments/dwlvt6/one_database_per_container_or_one_for_everything/

As said that isn't part of the discussion here.
But reading your thread, no it isn't, did you read that thread? Top comment discusses differences and benefits of single. Some other comments straight up say single is better. The thread is very split up with majority of upvotes swaying toward 1 large database.
You're giving the option to not use your database, therefore it shouldn't be running.
Why run more things that don't get used? That's what this issue thread is about.
Regardless postgresql has streaming replication and async / sync redundancies for offsite.


It should all "double complex* out" @hongzhouye @sunqm

double complex array can be viewed as double array that alternatively stores the real and imaginary parts. out array here can be a real array for integrals at gamma point or a complex array for other k-points. It may be not a good practice in the sense of software engineering to mix two different data types here. The code should work without any problems.


Add mermaid or other diagram tool to Doxygen.

I would love to add mermaid or another diagramming tool to Doxygen.
Very simular to how we added math rendering to Doxygen, I would like to have inline diagram-code in the markdown so that it will be rendered when displayed on the browser.

I've updated Doxygen to v1.9.8 and integrated Mermaid, but I don't know how to draw with it yet.
There are some changes in the new version:

The "modules" tab in the navigation was renamed to "topics." It appears that Doxygen will use "modules" for C++ modules.
The integration of markdown pages needed some config tweaks.
I've also made some CSS tweaks, adding more margin on the left side, aligning some title items, and changing the link visiting and hovering colors and effects.

The updated documentation can be viewed here:

Overview Page for API Docs: https://jakoch.github.io/hikogui.github.io/docs/
main - branch: https://jakoch.github.io/hikogui.github.io/docs/hikogui/main/

I'll send the pull request once I have the version_dropdown fixed and a working Mermaid example.

One issue remains: Due to the large number of feature branches and version numbers for which we generate API docs, we are approaching the GitHub storage limit for gh-pages (1GB).
Possible solutions:

My suggestion is to limit the version display to only include released versions (version tags) and exclude the branches.
Host website at a location with more storage space.

Maybe make a white list of which branches/releases should be rendered.
Is it simply all the html that is generated that causes the majority of diskspace?


[webview] let WebViewController support shouldInterceptRequest for loading offline resources

Use case
webview_flutter
Let the WebVIew's WebViewController support shouldInterceptRequest for loading offline resources.
In WebView loading optimization, local resources will be used to replace network resources to provide webview loading speed. In this case, use the shouldInterceptRequest of WebViewClient of Android WebView to replace resource request intercepting with local resources.
Proposal

Supporting this without a way of doing synchronous calls to Dart would be extremely cumbersome, so this would likely need to wait for if/when there is a solution for synchronously calling Dart code on the platform isolate.

I just noticed that this was already filed as https://github.com/flutter/flutter/issues/27896. Closing as a duplicate.


Use consistent version comparison even when comparing preferred versions

Reviewing cheatsheet
Before merging the PR, comments starting with

❌ ❓must be fixed
🤔 💅 should be fixed
💭 may be fixed
🎉 celebrate happy things

@bot-gradle merge

The merge queue build has started. Click here to see all failures if any.

The merge queue build has started. Click here to see all failures if any.


[PLUGIN REQUEST] nushell treesitter grammer

Field
Description

Plugin
tree-sitter-nu

Homepage
https://github.com/nushell/tree-sitter-nu/tree/main

Nixpkgs
https://search.nixos.org/packages?channel=23.11&show=tree-sitter-grammars.tree-sitter-nu&from=0&size=50&sort=relevance&type=packages&query=tree-sitter-nu

Extra Information

I don't think there is an easy way to enable this without the grammar being included into tree-sitter.
You can still add custom grammars, though. I am using this same grammar in my config here:
https://github.com/siph/nixvim-flake/blob/22166973ec14370830bfec5ee87c7bfcbbd3c946/config/treesitter/treesitter.nix
Here's the discussion about custom grammars: https://github.com/nix-community/nixvim/discussions/540
Note that you no longer need to build the nushell grammar because it's included in nixpkgs.

Here is what you need to do to get the nushell grammar in nixvim:
https://gist.github.com/GaetanLepage/7372ae30a27c40f2beb0019f1893f3b6

Yes, unfortunately we cannot do much here.
For the record, I will leave the final working configuration for adding nushell to nixvim: https://gist.github.com/GaetanLepage/7372ae30a27c40f2beb0019f1893f3b6
This is something you can do in your config, but we are not going to upstream it.
Of course, if the grammar gets added to treesitter, then we would defacto support it out of the box.


[Popover][FocusScope] Allow me to modify the default autofocus behavior of FocusScope

Feature request
Overview
By default PopoverContent cannot autofocus any <a> elements on open because of this commit. The <a> elements are filtered out from the tappable candidates.
How to prevent this behavior and include links into the tappable candidates without calling .preventDefault() in onOpenAutoFocus? That will require me to pull all of the util functions from FocusScope.tsx into my app because they are not exported.
Examples in other libraries

Who does this impact? Who is this for?
Myself
Additional context
If you would consider a PR that will allow me to prevent this behavior I could work on it myself.

Seems like its an intentional behavior. i will just pull or the util functions from FocusScope.tsx and override onOpenAutoFocus


Issue 48 plus commented dead code removed

Maybe move logger as top-level variable is a god idea

Sure. Top level in __init__?

Check style, it fails in cov also, but not that relevant yet.


There are some images in the dataset that have deviated labels.

Hi @R3ab,
There are some images in the dataset that have deviated labels. How can I fix them?

I will fix this images and update the dataset very soon. If you notice this issue in more than one image, please let me know the number of images.

@R3ab  Only 2 images: 76_3795.jpg and 76_3810.jpg

you can access the updated source of dataset.

thank you!


/zuul problem

Describe the question
Before my api-gateway built from Zuul, for upload file with prefix /zuul
Now I immigrate to Spring Cloud Gateway, a problem is, when upload file with prefix /zuul, just like before,
but return as below:
{
    "timestamp": "2020-07-03T08:29:06.648+0000",
    "path": "/zuul/v1/test/upload",
    "status": 404,
    "error": "Not Found",
    "message": null
}

how to solve this problem?
Sample
null

      routes:
        - id: test-v1
          uri: lb://test-v1
          predicates:
            - Path=/zuul/**