Add a BinariesPath configuration option, for prefetching Postgres binaries before the test

[mishas]

Downloading and Un-archiving the postgres binaries is slow and flaky in some CI systems.
For example - in my case, our CI system starts a fresh sandbox for every test it runs, and as such caching the downloaded postgres file is impossible.
Even when it is possible to cache the downloaded file, it's still might be slow due to the time it takes to unarchive it.

The change in this PR does the following:
It gives an option to specify a binaries path, where the binaries will be unarchived to, and will not get deleted between tests.
If the BinariesPath/bin directory exists, the system will assume files were pre-downloaded, and will not try to fetch/unarchive them.

The PR is split into 4 commits:

1. Refactor of the usage of the folders, in order to not "recalculate" the data/runtime directories over and over again (makes code simpler)
2. Separates the variable holding the runtimePath from the one holding the binaryExtractLocation (as those can now be different).
3. Adds the BinariesPath to the configuration, and adds the logic to reuse unarchived binaries (as well as adds new tests)
4. Updates README (documentation)

This change should be fully backward compatible if BinariesPath is not specified.

This also helps with #35 (@mbfr).

[mishas]

@fergusstrange , just making sure that you saw this PR.

Thanks.

[fergusstrange]

Sorry @mishas I totally did miss this! I've approved CI run and will review code this evening. Looks like a good change to include!

[mishas]

@fergusstrange , Thanks for looking into it!

I just fixed the lint warnings and the failing test from the CI, and pushed 2 fixing commit.

A little bit about the failing test (Test_ErrorWhenRemoteFetchError on the Alpine platform):

--- FAIL: Test_ErrorWhenRemoteFetchError (0.00s)
    embedded_postgres_test.go:74: 
        	Error Trace:	embedded_postgres_test.go:74
        	Error:      	Error message not equal:
        	            	expected: "did not work"
        	            	actual  : "unable to create runtime directory extracted with error: mkdir extracted: permission denied"
        	Test:       	Test_ErrorWhenRemoteFetchError

Before my change, the logic used to be:

1. Download the file if doesn't exist
2. Unarchive the file into the folder (which also created the folder if it doesn't exist)

In my change, I've split runtimePath from binariesPath, and now, (2) above only creates the binariesPath folder (if doesn't exist), but not the runtimePath.
To fix that, I've added os.MkdirAll(ep.config.runtimePath, 0755), and I put it right after os.RemoveAll(ep.config.runtimePath) to make the flow of the logic make sense.

The problem is - that the MkdirAll fails on the Alpine setup (there're no permissions to create any folders), and since it happens before the download, the MkdirAll fails, instead of the actual download.

To make things simple, I've fixed by moving the MkdirAll to after the download.

[fergusstrange]

Suggested change

	// Expect everything to work without cacheLocator and/orr remoteFetch abilities.
	// Expect everything to work without cacheLocator and/or remoteFetch abilities.

[fergusstrange]

@mishas changes all look sensible. One small typo but nothing major.

Looks like the Alpine tests may still be failing after moving it? We'll need to get this fixed up before we can move forward. I'll look into auto running these tests on push as well, I'm not sure why they've stopped...

Thanks

[mishas]

@fergusstrange ,

I fixed the test, and ran it in my repo: https://github.com/mishas/embedded-postgres/actions/runs/1233929433
Seems to pass now.

Thanks again.

[fergusstrange]

@mishas seems the Windows tests are now failing 🤦 around cleaning up after themselves.

remove C:\Users\RUNNER~1\AppData\Local\Temp\embedded_postgres_go_tests207810035\2\bin\postgres.exe: The process cannot access the file because it is being used by another process.

I'm concerned we've not got he logic quite correct here if we're getting conflicting processes accessing the files. Do we need to take a slightly different approach?

It's also interesting to see that the tests ran on your fork. Did you alter anything in the build? 🤔

[mishas]

@fergusstrange , this is really really strange!
I didn't change anything in the CI that I ran on my fork, and it passed.
Moreover - I didn't change anything that should've made a difference between the previous run (which failed on alpine, but passed on windows) and this run.

Furthermore, the test that is failing (platform-test/platform_test.go), is not affected by my change at all: we do not define binariesPath in that use-case, and as such - the old flow is happening.

So - what is happening?

I'm concerned we've not got he logic quite correct here if we're getting conflicting processes accessing the files.

First of all - this shouldn't matter at all - you may run the same executable from two different processes. Whenever we do reuse the binaries - they're separated from the data and the runfiles, and as such can be run multiple times (can even be readonly)
Am I missing something?

Is it possible it's some freakish race condition of the windows OS that's happening here? Can we try re-running the CI please?

[mishas]

@fergusstrange , I was able to reproduce the same failure on master: https://github.com/mishas/embedded-postgres/pull/2/checks?check_run_id=3605422559
(I created an empty PR, and ran the CI 2 times in a raw to get the same failure)

[Please note that the PR is not exactly empty, it has an extra space in README.md to make the CI actually trigger a new run...]

[fergusstrange]

Yeah this is very odd.

I've rerun an old build https://github.com/fergusstrange/embedded-postgres/runs/3606273031?check_suite_focus=true and receiving the same failures you saw on your forked CI.

Let me see if I can rewrite these tests quickly to be less flakey, then we can rebase on top of that and get this merged in.

[fergusstrange]

Okay @mishas I've dropped the removeAll which appears to have become flakey in Github Actions Windows only. This shouldn't be a problem as it's written to the OS Temp Dir anyway so would be cleaned up on reboot.

https://github.com/fergusstrange/embedded-postgres/pull/39/files

Try rebasing and see how that goes.

[mishas]

@fergusstrange ,
Thank you for that very quick turn-around on this PR!
I just rebased onto your master, and tests seem to pass now.

I can add tests to increase coverage if that's something you think is worth doing, but the missing coverage is for errors during os.MkdirAll and os.RemoveAll, which seem redundant to test.

[fergusstrange]

Great work @mishas!

I've merged and will just allow a little manual testing with some of my own projects quickly.

I'll cut a new release if this goes well tomorrow :)

Thanks

[mishas]

Thank you very much @fergusstrange ,
Really appreciate the very fast turn-around.

[fergusstrange]

Release available here https://github.com/fergusstrange/embedded-postgres/releases/tag/v1.10.0

Thanks @mishas