Skip to content

Remove use of filepath for interacting with embedded migration files - #485

Merged
brandur merged 1 commit into
masterfrom
brandur-remove-filepath
Aug 1, 2024
Merged

Remove use of filepath for interacting with embedded migration files#485
brandur merged 1 commit into
masterfrom
brandur-remove-filepath

Conversation

@brandur

@brandur brandur commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

This one is in pursuit of resolving #482 in which migrations can't
currently run on Windows.

As @bgentry points out regarding the embed package [1], where forward
slashes are used for all files regardless of underlying operation system:

The patterns are interpreted relative to the package directory
containing the source file. The path separator is a forward slash, even
on Windows systems.

But filepath uses OS-specific separators, making commingling use of
these two packages not advisable:

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.

Here, remove use of filepath for migrations by using the IsDir
function of a file entry instead, which also turns out to be much
cleaner anyway.

I don't have a Windows system to test on, but this seems like a pretty
likely fix. A Windows-specific job to test the CLI is in progress as
part of #483, which would help prevent future regressions.

Fixes #482.

[1] https://pkg.go.dev/embed#hdr-Directives

@brandur
brandur force-pushed the brandur-remove-filepath branch from b32ceb5 to 25391be Compare July 31, 2024 00:21
@brandur
brandur requested a review from bgentry July 31, 2024 00:30
@bgentry

bgentry commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

I just set up Windows CI for the CLI in #483, which reveals the exact issue this PR is supposed to fix. Let's merge that and rebase this one on top of it to confirm the fix?

@bgentry

bgentry commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

Unfortunately this does not appear to resolve the issue. I rebased my PR #483 on this one and did a single run, and a similar panic happened: https://github.com/riverqueue/river/actions/runs/10172817143/job/28135934181?pr=483

However, with the CI in that branch we can at least reproduce this somewhat easily. We can merge that and then you can push a branch with some debug output to help track this down. Not ideal, but neither is finding a real Windows machine to test on 😉

@brandur
brandur force-pushed the brandur-remove-filepath branch 5 times, most recently from 4bf5072 to afe8c66 Compare July 31, 2024 14:54
@brandur

brandur commented Jul 31, 2024

Copy link
Copy Markdown
Contributor Author

@bgentry So I tried this against your Window job, and it's a little unintuitive, but it actually does work. The problem is that along with this change you also need to uncomment the replace directives in cmd/river/go.mod so that the CLI there pulls in the version of rivermigrate that's on this branch (otherwise it's using the latest tagged version).

As we're sending out this fix we'll need to make sure to cut a new CLI release with bumped versions of the other River packages.

@bgentry

bgentry commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

@brandur sweet! This will just need a rebase to get the updated branch protection rules.

This one is in pursuit of resolving #482 in which migrations can't
currently run on Windows.

As @bgentry points out regarding the `embed` package [1], where forward
slashes are used for all files regardless of underlying operation system:

> The patterns are interpreted relative to the package directory
> containing the source file. The path separator is a forward slash, even
> on Windows systems.

But `filepath` uses OS-specific separators, making commingling use of
these two packages not advisable:

> 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.

Here, remove use of `filepath` for migrations by using the `IsDir`
function of a file entry instead, which also turns out to be much
cleaner anyway.

I don't have a Windows system to test on, but this seems like a pretty
likely fix. A Windows-specific job to test the CLI is in progress as
part of #483, which would help prevent future regressions.

Fixes #482.

[1] https://pkg.go.dev/embed#hdr-Directives
@brandur
brandur force-pushed the brandur-remove-filepath branch from afe8c66 to 366f3db Compare August 1, 2024 00:32
@brandur

brandur commented Aug 1, 2024

Copy link
Copy Markdown
Contributor Author

TY. The Windows build is still failing, but we should see it pass as I do the second phase of a new release.

@brandur
brandur merged commit e903376 into master Aug 1, 2024
@brandur
brandur deleted the brandur-remove-filepath branch August 1, 2024 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic when using cli migration

2 participants