Skip to content
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

Merged
merged 13 commits into from
Sep 20, 2023

Conversation

andrewwasielewski
Copy link
Contributor

@andrewwasielewski andrewwasielewski commented Feb 7, 2023

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

The primary changes are:

  • updating function prototypes with PGDLLEXPORT
  • adding pgsodium_getkey.bat for windows (assumes openssl is installed)
  • utilizing _access and a windows defined getline function to check the permissions and read the output of the getkey_script
  • adding a .vcxproj file for building on windows with msbuild

Copy link

@Godwottery Godwottery left a 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.

src/pgsodium.c Outdated Show resolved Hide resolved
src/pgsodium.c Outdated Show resolved Hide resolved
src/pgsodium.c Show resolved Hide resolved
src/pgsodium.c Outdated Show resolved Hide resolved
@michelp
Copy link
Owner

michelp commented Feb 10, 2023

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
Copy link

Godwottery commented Feb 14, 2023

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
Copy link

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link

Godwottery commented Mar 9, 2023

@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
Copy link
Contributor Author

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

@michelp
Copy link
Owner

michelp commented Apr 4, 2023

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
Copy link
Owner

michelp commented May 29, 2023

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
Copy link
Contributor Author

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

@michelp michelp merged commit dc411ab into michelp:main Sep 20, 2023
2 checks passed
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.

3 participants