fix: filesystemStore fails to prepend target file hashes on Windows

[torin-carey]

Fixes #273
Release Notes: Fix commit for Windows when using filesystemStore.

Types of changes:

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of the changes being introduced by the pull request:

The filesystemStore LocalStore implementation conflates paths and filepaths,
which cause issues on systems where they take different formats (Windows).
This is most noticeable when running a repository with consistent snapshots, where committed targets will not have a prepended file hash, since the filesystemStore implementation mistakes them for metadata files, rather than targets:

// local_store.go
func (f *fileSystemStore) Commit(consistentSnapshot bool, versions map[string]int, hashes map[string]data.Hashes) error {
	isTarget := func(path string) bool {
		return strings.HasPrefix(path, "targets/")
	}
	copyToRepo := func(path string, info os.FileInfo, err error) error {
		if err != nil {
			return err
		}
		if info.IsDir() || !info.Mode().IsRegular() {
			return nil
		}
		rel, err := filepath.Rel(f.stagedDir(), path)
		if err != nil {
			return err
		}

		var paths []string
		if isTarget(rel) {
			paths = computeTargetPaths(consistentSnapshot, rel, hashes)
		} else {
			paths = computeMetadataPaths(consistentSnapshot, rel, versions)
		}
		// ...
	}
	// ...
	if err := filepath.Walk(f.stagedDir(), copyToRepo); err != nil {
		return err
	}
	// ...

Because of this, clients will fail to find the target file from the repository.

Please verify and check that the pull request fulfills the following requirements:

• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

[znewman01]

This looks great, thanks!

2 things:

1. can you add Windows to the test matrix in .github/workflows/tests.yml? Should just be a matter of adding windows-latest here:

   go-tuf/.github/workflows/tests.yml

   Line 20 in 3dfbeb2

   os: [ubuntu-latest, macos-latest]

2. Test failure. I'm a little perplexed as to why this happens specifically on 1.17...it may be unrelated but I haven't seen it fail this way before.

[rdimitrov]

@torin-carey - It's important to have consistent support for Windows too, so thank you for helping with that! 💯

Perhaps it needs a bit more polishing though, because CI failed not only for 1.17, but for the whole set of test cases (it cancels the rest when one fails). You can try debugging these issues by running the test suite locally via go test ./...

Also, as @znewman01 suggested, adding the Windows environment to the CI config would be useful to make sure we support it continuously 👍

Partially related - #131, #79, #80

[torin-carey]

I've sorted the failing tests (bug in original commit causing files to not be deleted after removing target) and all of the go tests are passing on Linux and Windows. I've also replaced the use of filepath with path in (*DelegatedRole).MatchesPath since the escape semantics of filepath.Match and path.Match are different between Windows and *nix.

I've also added windows-latest to the workflows test matrix but haven't been able to test this with workflows in a separate repo, so 🤞

[znewman01]

Thank you very much! This looks fine to me barring the test failures (which might just be lint related?)

[znewman01]

Looking at the last test failures: I see

go-tuf/client/interop_test.go

Line 47 in ad96eca

c.Assert(hashes, DeepEquals, snapshotHashes, Commentf("metadata out of date, regenerate by running client/testdata/go-tuf/regenerate-metadata.sh"))

It looks like one of those is reading \r\n. Maybe related to actions/checkout#135 . I think that we could either turn autocrlf off in a test environment, or make the computeHashes test function normalize line endings. Maybe the latter is better so that tests will actually work in a windows source checkout.

[torin-carey]

Thanks for reviewing!

It looks like one of those is reading \r\n. Maybe related to actions/checkout#135 . I think that we could either turn autocrlf off in a test environment, or make the computeHashes test function normalize line endings. Maybe the latter is better so that tests will actually work in a windows source checkout.

Thanks for finding this, the line endings do seem to be the only issue affecting the windows tests. I don't think it would suffice to just change the reader, since the mangling of the metadata files by git really does infringe on their integrity and I don't think we should try and accommodate that. Adding .gitattributes to either mark all of the testdata as binary or have eol=lf I think would be the right approach.

The linter errors also seem to be about line endings, since gofmt wants to correct \r\n => \n (see golangci/golangci-lint#580). Maybe we should add a project-wide *.go eol=lf git attribute?

Another problem I've noticed running git on windows locally is that git for windows by default does not create symlinks, causing some of the test cases to fail where test case metadata files are de-duplicated through links. Github workflows seems to enable core.symlinks so this shouldn't be a problem for the CI, but may be for users. Should we remove the symlinks or just leave it as-is?

[znewman01]

I don't think it would suffice to just change the reader, since the mangling of the metadata files by git really does infringe on their integrity and I don't think we should try and accommodate that. Adding .gitattributes to either mark all of the testdata as binary or have eol=lf I think would be the right approach.

You've convinced me, I think that's correct.

The linter errors also seem to be about line endings, since gofmt wants to correct \r\n => \n (see golangci/golangci-lint#580). Maybe we should add a project-wide *.go eol=lf git attribute?

SGTM

Another problem I've noticed running git on windows locally is that git for windows by default does not create symlinks, causing some of the test cases to fail where test case metadata files are de-duplicated through links. Github workflows seems to enable core.symlinks so this shouldn't be a problem for the CI, but may be for users. Should we remove the symlinks or just leave it as-is?

Good find. I think we can address this later; I'll file an issue. The symlinks are all consistent snapshot-related so I think they're somewhat deliberate.

TBH I'd prefer to keep the symlinks and fix this with documentation. If we must remove them, we'd need some automation around making sure the files stay in sync.

[joshuagl]

Thanks for reviewing!

It looks like one of those is reading \r\n. Maybe related to actions/checkout#135 . I think that we could either turn autocrlf off in a test environment, or make the computeHashes test function normalize line endings. Maybe the latter is better so that tests will actually work in a windows source checkout.

Thanks for finding this, the line endings do seem to be the only issue affecting the windows tests. I don't think it would suffice to just change the reader, since the mangling of the metadata files by git really does infringe on their integrity and I don't think we should try and accommodate that. Adding .gitattributes to either mark all of the testdata as binary or have eol=lf I think would be the right approach.

💯 this is what we did for python-tuf: theupdateframework/python-tuf@69cc684

[znewman01]

1 more annoying thing, sorry: #319

(You'll need to rebase and do something like: https://github.com/src-d/guide/blob/master/developer-community/fix-DCO.md before you push again)

[znewman01]

@torin-carey anything the maintainers can do on our end to help out with this?

(No urgency, just want to make sure you're not blocked)

[torin-carey]

Hi @znewman01, I apologise for the delayed response.

The current state of the PR works for me with both golangci-lint and go test/check through a similar git config to workflows. Hopefully workflows agrees 🤞. Commits have also been signed off for DCO.

[znewman01]

No need to apologize; we appreciate the contribution!

I'm OOO this week so I'll take a look next week; if you'd prefer a quicker review, CC in a few of the other maintainers.

[joshuagl]

Thank you so much for your work on this! I'm looking forward to having Windows working and tested :-)

The PR looks great. I just have one small question about the gitattributes file.

[znewman01]

Oh, I'm taking @rdimitrov off as reviewer because he's currently on leave.

@joshuagl can you review for approval?

[joshuagl]

LGTM, thanks so much @torin-carey !

[torin-carey]

Thanks for reviewing and approving!