cmd/gitannex: Add layout modes for compatibility #7754

dmcardle · 2024-04-10T23:50:23Z

What is the purpose of this change?

This change adds support for the repo layouts defined by git-annex-remote-rclone. This should enable git-annex users with external remotes of type "rclone" to switch to "rclone-builtin" without needing to retransfer content. It also adds e2e compatibility tests in an attempt to verify this claim!

Was the change discussed in an issue or in the forum before?

Issue #7625

Checklist

I have read the contribution guidelines.
I have added tests for all changes in this PR if appropriate.
I have added documentation for the changes if appropriate.
All commit messages are in house style.
I'm done, this Pull Request is ready for review :-)

dmcardle · 2024-04-11T16:08:28Z

@ncw, PTAL!

FYI: @yarikoptic

dmcardle · 2024-04-11T19:47:04Z

Wow, the for loop thing bit me again. Fixed in the latest push. https://go.dev/blog/loopvar-preview

Edit: filed #7758 to start a discussion about upgrading to Go 1.22.

yarikoptic · 2024-04-12T17:04:19Z

This should enable git-annex users with external remotes of type "rclone" to switch to "rclone-builtin" without needing to retransfer content.

I haven't had a chance to try yet, but I just want to say Thank you. I wonder if there could be e2e test run where first repo is initialized to use git-annex-remote-rclone and then migrated to use git-annex-remote-rclone-builtin and ensured to still operate correctly -- can get a previously uploaded file and then annex testremote. That would be very much appreciated before I try to migrate thousands of repos (https://github.com/dandisets and https://github.com/dandizarrs) covering 100s of TBs of data already in dropbox ;)

dmcardle · 2024-04-15T14:49:33Z

I haven't had a chance to try yet, but I just want to say Thank you.

You're very welcome! I'm just working on this for fun, so it's a pleasant surprise that someone finds it useful!

I wonder if there could be e2e test run where first repo is initialized to use git-annex-remote-rclone and then migrated to use git-annex-remote-rclone-builtin and ensured to still operate correctly -- can get a previously uploaded file and then annex testremote.

In case you didn't spot it, the new TestEndToEndRepoLayoutCompat test in e2e_test.go is doing something kind of like this. It creates two remotes backed by the same local rclone remote, one externaltype=rclone and one externaltype=rclone-builtin. It copies files to the first remote and makes sure they're present on the second, etc.

You raise a good point, though. I think it's worth adding a migration test that runs git annex testremote after changing the externaltype of an existing remote. I think we can change externaltype with git annex enableremote, but I haven't tried it yet. This shouldn't be too hard, I'll try to squeeze it into this PR.

That would be very much appreciated before I try to migrate thousands of repos (https://github.com/dandisets and https://github.com/dandizarrs) covering 100s of TBs of data already in dropbox ;)

Looks like nearly ~~an exabyte~~ a petabyte! 🤯

dmcardle · 2024-04-15T17:00:43Z

@yarikoptic PTAL at the latest commit, which adds TestEndToEndMigration tests.

yarikoptic · 2024-04-15T19:22:29Z

test in e2e_test.go is doing something kind of like this. It creates two remotes backed by the same local rclone remote, one externaltype=rclone

that is great - thank you! I have only vague memory on how I decided that there is none -- I remember searching for git-annex-remote-rclone and finding none -- but I see it all over now, so I guess my "search foo" was weak at that point.
But I still do not see any hit for installation of git-annex-remote-rclone in .github/workflows:

❯ git grep git-annex-remote-rclone -- .github/ || echo empty
empty

aren't e2e tests ran on CI? if so -- should get git-annex-remote-rclone installed to exercise testing you introduced.

dmcardle · 2024-04-15T20:42:24Z

aren't e2e tests ran on CI? if so -- should get git-annex-remote-rclone installed to exercise testing you introduced.

Ah! Great catch! I'll update this PR to install git-annex-remote-rclone on Linux and macOS. (Still punting on Windows.)

dmcardle · 2024-04-15T20:54:29Z

@ncw PTAL :)

Also, is there a way to tell which tests were skipped on CI? What do you think about changing CI to run go test -v? It would produce a lot more output, at least for these new tests that run git annex testremote, but we'd still get a concise summary at the end.

The reason I ask is that I mistakenly believed these new tests were running on CI because I saw something like this in the log, but it turned out that they were certainly being skipped due to the missing git-annex-remote-rclone binary.

ok      github.com/rclone/rclone/cmd/gitannex   0.266s

dmcardle · 2024-04-17T13:11:14Z

Hey @ncw, looks like CI is skipping the gitannex end-to-end tests because the rclone binary on PATH has an unexpected version string. For example, from the linux job's log:

=== RUN   TestEndToEnd
    e2e_test.go:199: Skipping due to rclone version: expected version "v1.67.0-DEV", but got "v1.67.0-beta.7905.220bbe24d.merge"
--- SKIP: TestEndToEnd (0.07s)
=== RUN   TestEndToEndMigration
    e2e_test.go:199: Skipping due to rclone version: expected version "v1.67.0-DEV", but got "v1.67.0-beta.7905.220bbe24d.merge"
--- SKIP: TestEndToEndMigration (0.06s)
=== RUN   TestEndToEndRepoLayoutCompat
    e2e_test.go:199: Skipping due to rclone version: expected version "v1.67.0-DEV", but got "v1.67.0-beta.7905.220bbe24d.merge"
--- SKIP: TestEndToEndRepoLayoutCompat (0.07s)

I have been assuming that the value of fs.Version seen by tests would match the version in the newly-built rclone binary, but that's clearly not the case!

It looks like the tests construct "v.1.67.0-DEV" based on this logic:

rclone/fs/version.go

Lines 8 to 12 in 3051769

    
           if VersionSuffix == "" { 
        
           	Version = VersionTag 
        
           } else { 
        
           	Version = VersionTag + "-" + VersionSuffix 
        
           }

Meanwhile, the Makefile just entirely clobbers fs.Version:

rclone/Makefile

Line 53 in 3051769

    
           go build -v --ldflags "-s -X github.com/rclone/rclone/fs.Version=$(TAG)" $(BUILDTAGS) $(BUILD_ARGS)

The latest commit changes the Makefile to clobber the version tag for tests, too. Let's see if that does the trick :)

Makefile

dmcardle · 2024-04-17T13:43:21Z

Interesting, the tests seem to run on Linux now, but mac_amd64 now skips the tests for a different reason:

2024-04-17T13:12:28.1429840Z === RUN   TestEndToEnd
2024-04-17T13:12:28.2422770Z     e2e_test.go:200: Skipping due to rclone version: expected tag string "none", but got "cmount"
2024-04-17T13:12:28.2424190Z --- SKIP: TestEndToEnd (0.10s)
2024-04-17T13:12:28.2426030Z === RUN   TestEndToEndMigration

dmcardle · 2024-04-29T14:24:03Z

Interesting, the tests seem to run on Linux now, but mac_amd64 now skips the tests for a different reason:

2024-04-17T13:12:28.1429840Z === RUN   TestEndToEnd
2024-04-17T13:12:28.2422770Z     e2e_test.go:200: Skipping due to rclone version: expected tag string "none", but got "cmount"
2024-04-17T13:12:28.2424190Z --- SKIP: TestEndToEnd (0.10s)
2024-04-17T13:12:28.2426030Z === RUN   TestEndToEndMigration

Aha! I think what's happening is that CI is running something like GOTAGS=cmount make rclone and then make quicktest without any tags. The rclone binary thus tells us it was compiled with "cmount" tags, but the tests expect "none". @ncw is this behavior surprising to you or WAI?

As a workaround, I'll just log a warning rather than skipping e2e tests when the tags do not match.

dmcardle · 2024-05-02T14:18:09Z

@ncw This is ready for review, more or less!

I'm still stumped by the GoTags not matching, but my workaround is to just run the e2e tests anyway.

Another questionable change in this PR is that I changed go test commands to go test -v. Totally happy to remove it, but it was necessary for me to figure out whether the gitannex e2e tests were actually running in CI.

Issue rclone#7625

This commit adds support for the same repo layouts supported by git-annex-remote-rclone. This should enable git-annex users with remotes of type "rclone" to switch to a "rclone-builtin" without needing to retransfer content. Issue rclone#7625

TestEndToEndRepoLayoutCompat exercises git-annex-remote-rclone-builtin and git-annex-remote-rclone on the same rclone remote to ensure they are compatible. It repeats the same test for all known layout modes. Issue rclone#7625

I'm hopeful that running these in parallel will not impact CI runtime very much, but that likely depends on the number of CPU cores and whether the tmp filesystem is backed by memory vs a physical disk. Issue rclone#7625

Now that e2e tests are running in parallel, undoing the chdir to the temp dir was causing flaky failures on cleanup. We don't need it anyway because the worrisome subcommands have their working directory controlled by `runInRepo()`. Issue rclone#7625

Issue rclone#7625

For each layout mode, these tests start with a git-annex-remote-rclone remote, migrate it to a git-annex-remote-rclone-builtin remote. They verify that a file copied pre-migration is still present and that `git annex testremote` passes. Issue rclone#7625

Issue rclone#7625

dmcardle · 2024-05-13T13:36:04Z

@ncw Sorry to nag, but PTAL!

ncw

I had a look through that - all looking very nice.

Can you drop the commit adding -v to make quicktest and I'll merge what we've got - thank you :-)

ncw · 2024-05-13T16:41:31Z

Makefile

@@ -87,13 +87,13 @@ test:	rclone test_all

 # Quick test
 quicktest:
-	RCLONE_CONFIG="/notfound" go test $(BUILDTAGS) ./...
+	RCLONE_CONFIG="/notfound" go test -v $(BUILDTAGS) ./...


I'm not in favour of adding the -v here. Rclone has a lot of tests and having to look through the -v output in the CI is not fun! (279 lines vs 68057) So I'm not in favour of making it longer.

That's totally fair! It's not too hard to temporarily add the -v as needed. Done.

This enables gitannex end-to-end tests to run on CI. Otherwise, the version would not match and tests that check the rclone version would fail like so: ``` === RUN TestEndToEnd e2e_test.go:199: Skipping due to rclone version: expected version "v1.67.0-DEV", but got "v1.67.0-beta.7905.220bbe24d.merge" --- SKIP: TestEndToEnd (0.07s) ``` Issue rclone#7625

Issue rclone#7625

ncw

Perfect - thank you :-)

Will merge now.

dmcardle force-pushed the git-annex-layout-mode branch from 306ccee to 87a52e3 Compare April 11, 2024 15:59

dmcardle marked this pull request as ready for review April 11, 2024 16:07

dmcardle force-pushed the git-annex-layout-mode branch 2 times, most recently from f49d1b9 to 46e70f3 Compare April 11, 2024 19:38

dmcardle mentioned this pull request Apr 11, 2024

Upgrade to Go 1.22 to avoid scoping bugs with closures in for loops? #7758

Closed

dmcardle force-pushed the git-annex-layout-mode branch from f1e63ca to 85c3cf0 Compare April 15, 2024 17:38

dmcardle force-pushed the git-annex-layout-mode branch from 4120ee7 to 5742730 Compare April 17, 2024 12:30

dmcardle commented Apr 17, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

dmcardle force-pushed the git-annex-layout-mode branch from d57500d to 0285c95 Compare April 18, 2024 13:02

dmcardle mentioned this pull request Apr 18, 2024

cmd/gitannex: Add the gitannex command #7654

Merged

5 tasks

dmcardle force-pushed the git-annex-layout-mode branch from 0285c95 to c2d0ef0 Compare April 29, 2024 13:40

dmcardle force-pushed the git-annex-layout-mode branch from a5a9061 to 79236d8 Compare April 30, 2024 14:07

dmcardle added 6 commits May 13, 2024 09:34

cmd/gitannex: Simplify messageParser's finalParameter() func

b3590d2

Issue rclone#7625

cmd/gitannex: Refactor e2e tests, add layout compat tests

ed72e48

TestEndToEndRepoLayoutCompat exercises git-annex-remote-rclone-builtin and git-annex-remote-rclone on the same rclone remote to ensure they are compatible. It repeats the same test for all known layout modes. Issue rclone#7625

cmd/gitannex: Repeat TestEndToEnd for all layout modes

8734d75

I'm hopeful that running these in parallel will not impact CI runtime very much, but that likely depends on the number of CPU cores and whether the tmp filesystem is backed by memory vs a physical disk. Issue rclone#7625

cmd/gitannex: Describe new rclonelayout config in help

ba40332

Issue rclone#7625

dmcardle added 2 commits May 13, 2024 09:34

.github/workflows: Install git-annex-remote-rclone on Linux and macOS

019f91b

Issue rclone#7625

dmcardle force-pushed the git-annex-layout-mode branch from 79236d8 to 8bb03c6 Compare May 13, 2024 13:35

ncw reviewed May 13, 2024

View reviewed changes

dmcardle added 3 commits May 13, 2024 12:49

cmd/gitannex: Remove assumption in e2e test version check

1cbf9a2

cmd/gitannex: When tags do not match, run e2e tests anyway

d2196bf

Issue rclone#7625

dmcardle force-pushed the git-annex-layout-mode branch from 8bb03c6 to d2196bf Compare May 13, 2024 16:51

ncw approved these changes May 13, 2024

View reviewed changes

ncw merged commit f26fc8f into rclone:master May 13, 2024
9 of 10 checks passed

dmcardle mentioned this pull request May 13, 2024

New subcommand that allows exports from git-annex? #7625

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/gitannex: Add layout modes for compatibility #7754

cmd/gitannex: Add layout modes for compatibility #7754

dmcardle commented Apr 10, 2024 •

edited

dmcardle commented Apr 11, 2024

dmcardle commented Apr 11, 2024 •

edited

yarikoptic commented Apr 12, 2024

dmcardle commented Apr 15, 2024 •

edited

dmcardle commented Apr 15, 2024

yarikoptic commented Apr 15, 2024

dmcardle commented Apr 15, 2024

dmcardle commented Apr 15, 2024

dmcardle commented Apr 17, 2024

dmcardle commented Apr 17, 2024

dmcardle commented Apr 29, 2024

dmcardle commented May 2, 2024

dmcardle commented May 13, 2024

ncw left a comment

ncw May 13, 2024

dmcardle May 13, 2024

ncw left a comment

cmd/gitannex: Add layout modes for compatibility #7754

cmd/gitannex: Add layout modes for compatibility #7754

Conversation

dmcardle commented Apr 10, 2024 • edited

What is the purpose of this change?

Was the change discussed in an issue or in the forum before?

Checklist

dmcardle commented Apr 11, 2024

dmcardle commented Apr 11, 2024 • edited

yarikoptic commented Apr 12, 2024

dmcardle commented Apr 15, 2024 • edited

dmcardle commented Apr 15, 2024

yarikoptic commented Apr 15, 2024

dmcardle commented Apr 15, 2024

dmcardle commented Apr 15, 2024

dmcardle commented Apr 17, 2024

dmcardle commented Apr 17, 2024

dmcardle commented Apr 29, 2024

dmcardle commented May 2, 2024

dmcardle commented May 13, 2024

ncw left a comment

Choose a reason for hiding this comment

ncw May 13, 2024

Choose a reason for hiding this comment

dmcardle May 13, 2024

Choose a reason for hiding this comment

ncw left a comment

Choose a reason for hiding this comment

dmcardle commented Apr 10, 2024 •

edited

dmcardle commented Apr 11, 2024 •

edited

dmcardle commented Apr 15, 2024 •

edited