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

Add support for Visual Studio build #5874

Merged
merged 1 commit into from
Sep 19, 2021

Conversation

smlu
Copy link
Contributor

@smlu smlu commented Dec 29, 2020

This PR adds CMake build support for Visual Studio 2017 and newer. This build system doesn't require MSYS2 to be installed or used on Windows to successfully compile. All required dependency can be build using Visual Studio with exception to libgcrypt and libgpg-error for which a port for VS has to be used. For example, ShiftMediaProject port for libgcrypt and libgpg-error.
All KeePassXC modules successfully compile, run and work using this build system on Windows (e.g.: browser, sshagent, autotype, networking, updatecheck, keeshare). Please note, sshagent future was tested in app but was not tested with actually ssh-agent. The only not tested module is yubikey. I don't posses a YubiKey at the moment so I decided to skip this module and a future modification might be needed in order for this module to compile and/or run on Windows using this build system.
This build system also compiles with additional security compile flags /DYNAMICBASE, /guard:cf, /GS and linker flags /NXCOMPAT & /guard:cf when in release mode. This fixes issue #4582.

On the code side, a slight modification of existing CMake project and some of the .cmake modules had to be made to account for MSVC compiler and specific things related to building with VS compiler. The source code of KeeePassXC was not refactored much, only added or removed some of windows related macros here and there.
The biggest modification done to the CMake projects that also effects other build systems was made to Findsodium.cmake module and how libsodium is linked into the project. On Windows a macro SODIUM_STATIC has to be defined to properly import symbols from static library but at the moment this macro is not defined by current build system. Because Findsodium.cmake module already defines SODIUM_STATIC macro for sodium package a decision was made to refactor all CMake projects to link against sodium library via sodium package, opposite to linking directly to library constant ${sodium_LIBRARY_RELEASE}. This change successfully then links libsodium library and defines all necessary macros. Please note, due to constants sodium_LIBRARY_DEBUG and sodium_DLL_DEBUG were not used anywhere in the project, they were removed from Findsodium.cmake, and constants sodium_LIBRARY_RELEASE & sodium_DLL_RELEASE were renamed to sodium_LIBRARY and sodium_DLL.

fix #4582

Update: ported to #6209 to use Botan crypto library.

Testing strategy

Tests were run only for Visual Studio compiled builds and not for MinGW or other builds.
All test have passed successfully for debug and release mode.
Note: For some tests warnings were encountered.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)
  • ✅ Refactor (significant modification to existing code)

@droidmonkey
Copy link
Member

Awesome!

@phoerious
Copy link
Member

Fantastic. I hope that also makes it easier to use the newer Windows API features, which are not available in windows.h.

@smlu
Copy link
Contributor Author

smlu commented Dec 30, 2020

My next plan is to explore windows CNG API and see if any of possible storage providers (SW, TPM, Windows Hello) can be used to store master key (as some of KeePass plugins do). An later, try to use CNG API to load master key in and decrypt DB without exposing master key in KeePassXC memory.

Btw, I see 2 checks have failed should I try to fix them?

@droidmonkey
Copy link
Member

Run make format. Ignore the Linux one, random failure.

@droidmonkey
Copy link
Member

droidmonkey commented Dec 31, 2020

Need to include instructions on how to use vcpkg to provide most of the dependencies. I am getting very serious about replacing libgcrypt, libsodium (to a lesser extent), and Argon2 with Botan.

@smlu
Copy link
Contributor Author

smlu commented Jan 1, 2021

I am getting very serious about replacing libgcrypt, libsodium (to a lesser extent), and Argon2 with Botan.

Thumbs up for this! Botan is great c++ cryptography library. And having less dependency to worry about is I guess plus from security stand point. My next issue was just about asking why any of the well known c++ cryptography library like Botan or CryptoPP wasn't used in this project :)

@droidmonkey
Copy link
Member

droidmonkey commented Jan 1, 2021

I toyed with it this morning and am committing to Botan. It's about time and there is broad support in all package managers except Xenial, but a PPA can cover that case.

@droidmonkey droidmonkey added this to the v2.7.0 milestone Jan 15, 2021
@droidmonkey
Copy link
Member

This depends on #6209 merge

@smlu
Copy link
Contributor Author

smlu commented Mar 9, 2021

I'll update this PR once #6209 is merged.

@droidmonkey
Copy link
Member

Use a rebase

@droidmonkey
Copy link
Member

I have not forgotten about this. I think we're ready to incorporate.

@droidmonkey
Copy link
Member

One more blocker on this one, Yubikey libraries are not available in vcpkg. Going to be removing that dependency anyway since they are deprecated by Yubikey and I want to have a better API when access the keys. Plan is to move the relevant code directly into KPXC.

@droidmonkey
Copy link
Member

#6654 is posted, once merged we can get visual studio merged. This PR will need a setup guide included to get vcpkg and everything else squared away.

@smlu
Copy link
Contributor Author

smlu commented Jun 21, 2021

Once the #6654 is merged, I'll try to squeeze in the setup docs as soon as I'll be able to. Somebody else will also probably have to write the guidance for yubikey part (I still haven't got one).

@droidmonkey
Copy link
Member

Yubikey will work without any special actions after the other pr is merged. I am currently trying to fix the install command, msvc is no playing nice with cmake bundle tools.

@smlu
Copy link
Contributor Author

smlu commented Jun 30, 2021

Oh, that's great. Unfortunately, I'm a little bit busy at the moment, but please let me know if I can help you with anything.

@droidmonkey
Copy link
Member

Same here, we'll get there

@droidmonkey droidmonkey force-pushed the vs_build branch 2 times, most recently from 8a747b6 to 9aade70 Compare September 7, 2021 02:32
@droidmonkey
Copy link
Member

droidmonkey commented Sep 7, 2021

Whewww finally got everything working!

The magic variable to install dependencies is -DX_VCPKG_APPLOCAL_DEPS_INSTALL=ON

Need to incorporate msvc into the release tool and then this can be merged.

@droidmonkey droidmonkey force-pushed the vs_build branch 2 times, most recently from 172e1c1 to bf66fae Compare September 7, 2021 02:48
@smlu
Copy link
Contributor Author

smlu commented Sep 10, 2021

Fantastic! I'll try to find some time this weekend and test final version.

@droidmonkey
Copy link
Member

droidmonkey commented Sep 17, 2021

Release tool updated, we are good to go now!

./release-tool build -v 2.7.0 --snapshot --vcpkg "/c/vcpkg/scripts/buildsystems/vcpkg.cmake" --cmake-generator "Visual Studio 16 2019" -m "-m"

* Use C++17 when using MSVC compiler
* Remove unneeded header files and macros
* Removed unnecessary Yubikey cmake file
* Enhance release tool
* Updated INSTALL.md
Copy link
Member

@droidmonkey droidmonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished up the last minor issues when building a release package. This is ready to merge.

@droidmonkey droidmonkey merged commit 0c6587b into keepassxreboot:develop Sep 19, 2021
@smlu
Copy link
Contributor Author

smlu commented Oct 1, 2021

Finally got the chance to fully test the final (already merged) version of this PR.
I had no problem building the release and debug version using dependency libraries from the system but found 1 bug in FindQuaZip.cmake when using vcpkg package manager. The vcpkg installs quazip as quazip5.lib so the the call to find_library in FindQuaZip.cmake should look for names libquazip5 and quazip5. Also in vcpkg case when running compiled KPXC I had to rename quazip5.dll to quazip1-qt5.dll.

@droidmonkey
Copy link
Member

I am removing quazip in a future PR that is why I left it alone.

@smlu
Copy link
Contributor Author

smlu commented Oct 4, 2021

Oh, I wasn't aware of that.
Ok other than that everything works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compile Windows version with CFG enabled
3 participants