-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support building pgsodium on windows #72
Support building pgsodium on windows #72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a quick look through and made some minor comments.
If it were my project I would appreciate two pull requests, one that just added PGDLLEXPORT, which can be used by other OSes as well, and then one that adds on the Windows specific parts. That makes it easier to keep apart.
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. |
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. 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. |
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) |
@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. |
…latform toolset - Unit tests do not yet run, requires pgtap
@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? |
I have no further feedback at this time. |
@michelp Just checking in to see if you have any further comments or feedback? Thank you |
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. |
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 |
This PR adds support to build pgsodium on windows with msbuild. Addresses issue #37.
The primary changes are:
PGDLLEXPORT
pgsodium_getkey.bat
for windows (assumes openssl is installed)_access
and a windows definedgetline
function to check the permissions and read the output of thegetkey_script
.vcxproj
file for building on windows withmsbuild