fix: embedded fs on windows

[Dissociable]

Fixes #536

[dossy]

I'm a bit confused as to how the use of filepath.Clean results in the wrong behavior, as the entire sales pitch for filepath is that the utilities are supposed to Do The Right Thing™ according to the platform being used:

https://pkg.go.dev/path/filepath

The filepath package uses either forward slashes or backslashes, depending on the operating system. To process paths such as URLs that always use forward slashes regardless of the operating system, see the path package.

As readMigrationsDir is operating on filesystem paths and not URLs, normalizing forward slashes to backslashes when executing on Windows seems like the exact behavior that would be required here ... but, apparently that's incorrect, and you've got the right answer.

Could you add a automated test that, without your change, passes on Linux but fails on Windows, to clearly illustrate the issue, and hopefully prevent regressions in the future, as I can absolutely see someone reverting this change unintentionally, thinking that the use of path.Clean is wrong for cross-platform use and replace it with filepath.Clean again.

I'm mostly concerned that this is not a defect in dbmate, and that the use of filepath utils is correct cross-platform, and that you are actually running into a bug in go's embed, and specifically embed.FS on Windows not using the correct path separator:

The path separator is a forward slash, even on Windows systems.

This behavior is confusing, and is a specific and intentional behavior of Go's FS abstraction. It provides a uniform interface to filesystem paths on top of the underlying OS's filesystem, which may not be what you would expect if you don't read the documentation carefully.

Sadly, it seems like io.FS and embed.FS are sticking to their guns and insisting on this behavior: golang/go#44305

You should use path.Join, not filepath.Join for embed filenames.

And, we have this: https://pkg.go.dev/io/fs#ValidPath

Note that paths are slash-separated on all systems, even Windows. Paths containing other characters such as backslash and colon are accepted as valid, but those characters must never be interpreted by an FS implementation as path element separators.

So, yeah, it looks like your change is correct and path/filepath should not be used when using the FS filesystem layer abstraction. Confusing, I know ...

Anyway, could you add a test case that fails on Windows without the change in this PR? I think that would help ensure we don't regress this by accident in the future through some well-meaning change.

[Dissociable]

Very good review @dossy , thanks!

Regarding the test you asked, I may have not correctly understood you but the TestFindMigrationsFS test in pkg/dbmate/db_test.go already fails on Windows without merging this PR.

[dossy]

Regarding the test you asked, I may have not correctly understood you but the TestFindMigrationsFS test in pkg/dbmate/db_test.go already fails on Windows without merging this PR.

Perfect! Can you tell I've never run dbmate or its test suite on Windows, myself? :-)

I tried to build dbmate on Windows using Cygwin and MSYS2/ucrt64, and I get this error trying to build:

$ make build
rm -rf dist
go build -o dist/dbmate.exe -tags sqlite_omit_load_extension,sqlite_json -ldflags '-s'  .
# runtime/cgo
gcc_libinit_windows.c: In function ‘_cgo_beginthread’:
gcc_libinit_windows.c:143:27: error: implicit declaration of function ‘_beginthread’; did you mean ‘_cgo_beginthread’? [-Werror=implicit-function-declaration]
  143 |                 thandle = _beginthread(func, 0, arg);
      |                           ^~~~~~~~~~~~
      |                           _cgo_beginthread
cc1: all warnings being treated as errors
make: *** [Makefile:34: build] Error 1

How were you able to build dbmate on Windows?

[dossy]

Answering my own question, I installed the mingw-w64-ucrt-x86_64-toolchain package in MSYS2, and then I was able to build dbmate successfully. Now, to try and run some of the test suite without having Docker: my Windows environment is a VMware VM ... I don't want to even try running Docker inside a Windows VMware VM ...

[dossy]

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads
$ git clone https://github.com/amacneil/dbmate.git
Cloning into 'dbmate'...
remote: Enumerating objects: 2169, done.
remote: Counting objects: 100% (2169/2169), done.
remote: Compressing objects: 100% (896/896), done.
remote: Total 2169 (delta 1213), reused 2054 (delta 1166), pack-reused 0
Receiving objects: 100% (2169/2169), 2.72 MiB | 15.90 MiB/s, done.
Resolving deltas: 100% (1213/1213), done.

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads
$ cd dbmate

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads/dbmate
$ git status
On branch main
Your branch is up to date with 'origin/main'.

nothing to commit, working tree clean

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads/dbmate
$ git log -1
commit ea897783a09b7fec2eb82627bcfbab989f3429b8 (HEAD -> main, tag: v2.14.0, origin/main, origin/HEAD)
Author: Adrian Macneil <adrian@foxglove.dev>
Date:   Thu Mar 28 09:19:16 2024 -0700

    v2.14.0 (#534)

    - v2.14.0
    - Update dependencies

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads/dbmate
$ make build
rm -rf dist
go build -o dist/dbmate.exe -tags sqlite_omit_load_extension,sqlite_json -ldflags '-s'  .

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads/dbmate
$ make test POSTGRES_TEST_URL="postgres://postgres:postgres@postgres:15432/dbmate_test?sslmode=disable" FLAGS+="-v -run TestFindMigrationsFS ./pkg/dbmate/db_test.go #"
go test -p 1 -tags sqlite_omit_load_extension,sqlite_json -ldflags '-s' -v -run TestFindMigrationsFS ./pkg/dbmate/db_test.go # ./...
=== RUN   TestFindMigrationsFS
Dropping: dbmate_test
Creating: dbmate_test
    db_test.go:598:
                Error Trace:    C:/Users/dossy/Downloads/dbmate/pkg/dbmate/db_test.go:598
                Error:          Received unexpected error:
                                could not find migrations directory `./db/migrations`
                Test:           TestFindMigrationsFS
--- FAIL: TestFindMigrationsFS (0.32s)
=== RUN   TestFindMigrationsFSMultipleDirs
Dropping: dbmate_test
Creating: dbmate_test
    db_test.go:647:
                Error Trace:    C:/Users/dossy/Downloads/dbmate/pkg/dbmate/db_test.go:647
                Error:          Received unexpected error:
                                could not find migrations directory `./db/migrations_a`
                Test:           TestFindMigrationsFSMultipleDirs
--- FAIL: TestFindMigrationsFSMultipleDirs (0.40s)
FAIL
FAIL    command-line-arguments  0.960s
FAIL
make: *** [Makefile:43: test] Error 1

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads/dbmate
$ git remote add Dissociable https://github.com/Dissociable/dbmate.git

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads/dbmate
$ git fetch Dissociable
remote: Enumerating objects: 10, done.
remote: Counting objects: 100% (10/10), done.
remote: Compressing objects: 100% (5/5), done.
remote: Total 10 (delta 5), reused 10 (delta 5), pack-reused 0
Unpacking objects: 100% (10/10), 1.55 KiB | 20.00 KiB/s, done.
From https://github.com/Dissociable/dbmate
 * [new branch]      fix/embeddedfs_windows -> Dissociable/fix/embeddedfs_windows
 * [new branch]      main                   -> Dissociable/main

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads/dbmate
$ git checkout -b fix/embeddedfs_windows --track Dissociable/fix/embeddedfs_windows
branch 'fix/embeddedfs_windows' set up to track 'Dissociable/fix/embeddedfs_windows'.
Switched to a new branch 'fix/embeddedfs_windows'

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads/dbmate
$ git log ...main
commit cee57e0337f490013f7648a2c5e347334f6ea1e0 (HEAD -> fix/embeddedfs_windows, Dissociable/fix/embeddedfs_windows)
Author: Dissociable <50553790+Dissociable@users.noreply.github.com>
Date:   Tue Apr 9 14:05:37 2024 +0000

    fix: `db.FindMigrations()` now uses `path.Join` instead of `filepath.Join`

    to keep up the embedded fs compatibility on windows

commit 999f9f4108914cbd5014ca6e5b271a5edcd93654
Author: Dissociable <50553790+Dissociable@users.noreply.github.com>
Date:   Tue Apr 9 13:56:31 2024 +0000

    fix: embedded fs on windows

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads/dbmate
$ make build
rm -rf dist
go build -o dist/dbmate.exe -tags sqlite_omit_load_extension,sqlite_json -ldflags '-s'  .

dossy@DESKTOP-E99FK4U UCRT64 /c/Users/dossy/Downloads/dbmate
$ make test POSTGRES_TEST_URL="postgres://postgres:postgres@postgres:15432/dbmate_test?sslmode=disable" FLAGS+="-v -run TestFindMigrationsFS ./pkg/dbmate/db_test.go #"
go test -p 1 -tags sqlite_omit_load_extension,sqlite_json -ldflags '-s' -v -run TestFindMigrationsFS ./pkg/dbmate/db_test.go # ./...
=== RUN   TestFindMigrationsFS
Dropping: dbmate_test
Creating: dbmate_test
--- PASS: TestFindMigrationsFS (0.42s)
=== RUN   TestFindMigrationsFSMultipleDirs
Dropping: dbmate_test
Creating: dbmate_test
--- PASS: TestFindMigrationsFSMultipleDirs (0.40s)
PASS
ok      command-line-arguments  1.046s

LGTM 👍

[dossy]

Just for completeness, dbmate still works with the real filesystem, when using the OS-specific backslash path separator:

C:\Users\dossy\Downloads\dbmate>dist\dbmate.exe -u "postgres://postgres:postgres@postgres:15432/dbmate_test?sslmode=disable" -d testdata\db\migrations up -v
Applying: 20151129054053_test_migration.sql
Rows affected: 1
Applying: 20200227231541_test_posts.sql
Rows affected: 0

C:\Users\dossy\Downloads\dbmate>dist\dbmate.exe -u "postgres://postgres:postgres@postgres:15432/dbmate_test?sslmode=disable" -d testdata\db\migrations down -v
Rolling back: 20200227231541_test_posts.sql
Rows affected: 0

C:\Users\dossy\Downloads\dbmate>dist\dbmate.exe -u "postgres://postgres:postgres@postgres:15432/dbmate_test?sslmode=disable" -d testdata\db\migrations down -v
Rolling back: 20151129054053_test_migration.sql
Rows affected: 0

C:\Users\dossy\Downloads\dbmate>dist\dbmate.exe -u "postgres://postgres:postgres@postgres:15432/dbmate_test?sslmode=disable" -d testdata\db\migrations down -v
Error: can't rollback: no migrations have been applied

[amacneil]

Is there a test that could be added that would run on linux and prevent regression in the future? Or do we need to run CI on windows in order to replicate this?

[dossy]

Is there a test that could be added that would run on linux and prevent regression in the future? Or do we need to run CI on windows in order to replicate this?

I don't think it would be possible on Linux, or any OS that doesn't use or at least recognize \ as a directory path separator.

[amacneil]

Ok, I added CI tests for Windows in #543, and demonstrated in #545 that the tests failed prior to this PR.