michelp / pgsodium

Modern cryptography for PostgreSQL using libsodium.
Other
555 stars 32 forks source link

Support building pgsodium on windows #72

Closed andrewwasielewski closed 1 year ago

andrewwasielewski commented 1 year ago

This PR adds support to build pgsodium on windows with msbuild. Addresses issue #37.

The primary changes are:

michelp commented 1 year ago

Having not touched a Windows machine for 27 years, I can't really speak to the code, but the changes are minimal and I'm inclined to merge it since the tests pass, but, I think there's one missing piece which is changes to the github action test runner that builds and runs the tests on Windows. Since I can't personally maintain it I would need that step done to ensure future releases on Windows aren't broken.

Godwottery commented 1 year ago

I looked into building with less requirements, I was particularly trying to get rid of the requirements to install Visual Studio on the build machine.

The entire project can be built using only cl.exe and the linker. This means it can also be built using msbuild. msbuild is available on the Microsoft homepage: https://visualstudio.microsoft.com/downloads/

I created a simple .proj file (attached as .proj.txt because Github wouldn't let me upload the .proj file inline), and can now build the dll from msbuild directly from the command line.

pgsodium.proj.txt

msbuild pgsodium.proj /p:libsodiumLocation="..Downloads\libsodium-1.0.18-stable-msvc\libsodium" /p:PostgreSQLLocation="C:\ProgramFiles\PostgreSQL\15"/p:Platform=x64 /p:Configuration=Release

I think this would be a simpler solution: there are less Windows specific files (only 1), the dependencies are smaller (msbuild < Visual Studio). The parametrization is also done on the command line, so the Windows readme can be simplified.

Note: I haven't checked the resulting dll, but it looks OK.

Godwottery commented 1 year ago

Built a dll using msbuild with the pgsodium.proj file only. Running that dll. Thus we can remove two of the three WIndows specific files, leaving only a simple build project (and perhaps the readme)

andrewwasielewski commented 1 year ago

@Godwottery Thanks for assembling the project file for msbuild. It works quite nicely and removes the manual steps required. I'm looking into creating a windows test runner with it.

andrewwasielewski commented 1 year ago

@michelp I've added a github actions test runner which builds and runs the unit tests on windows. Currently, the windows unit tests do not pass, however, the windows test output aligns with the linux test output (linux failure may possibly be masked since it's run inside a container?)

@Godwottery Thanks for all your input! Just checking to see if you have any further feedback? Would you like to mark our conversations as resolved, or should I do that?

Godwottery commented 1 year ago

@Godwottery Thanks for all your input! Just checking to see if you have any further feedback? Would you like to mark our conversations as resolved, or should I do that?

I have no further feedback at this time. Please go ahead and mark our conversations as resolved if you consider them done.

andrewwasielewski commented 1 year ago

@michelp Just checking in to see if you have any further comments or feedback? Thank you

michelp commented 1 year ago

Sorry for the delay, currently in the middle of another project rolling out, and will get to this PR in the next couple of days. One minor complication may be that recently @ioguix pointed out that the tests don't run properly with psql but should be using pg_prove, so I'm going to try and fix that which will likely mean some small changes to your PR to use pg_prove as well. Once I have a PR for that I'll ping that back here to make the necessary changes.

michelp commented 1 year ago

Hi @andrewwasielewski , sorry for the long delay! I just released 3.1.7 that now correctly uses pg_prove to run the tests and thus catches missing failures that were occuring that didn't get caught during debug, if you update to the latest main branch, I'll go through your PR again and merge it, thanks! Any questions just lmk.

andrewwasielewski commented 1 year ago

Hi @andrewwasielewski , sorry for the long delay! I just released 3.1.7 that now correctly uses pg_prove to run the tests and thus catches missing failures that were occuring that didn't get caught during debug, if you update to the latest main branch, I'll go through your PR again and merge it, thanks! Any questions just lmk.

Ok, thanks! Will update my PR with the latest changes and test