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

Refactor: move to simple default builds #11003

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

droidmonkey
Copy link
Member

@droidmonkey droidmonkey commented Jun 29, 2024

  • Remove individual feature flags in favor of a single KPXC_MINIMAL flag that removes advanced features from the build. Basic features are no longer guarded by feature flags.
  • Basic features: Auto-Type, Yubikey, KeeShare
  • Advanced features include: Browser (and passkeys), SSH Agent, and Secret Service
  • Networking, Documentation, and Update Checking remain as feature flags to accommodate various distro requirements.

This change also cleans up the main CMakeLists.txt by re-arranging some content and placing macros into a dedicated include file. The minimum CMake version was bumped to 3.16.0 to conform to our minimum Ubuntu support of Focal (20.04). This also allows us to default to C++20, we fall back to C++17 for Qt versions less than 5.15.0 due to lack of support.

Lastly this change removes the KEEPASSXC_BUILD_TYPE="PreRelease" which is never used. We only support "Snapshot" and "Release" now.

Testing strategy

Tested locally and with existing CI

Type of change

  • ✅ Breaking change (causes existing functionality to change)
  • ✅ Refactor (significant modification to existing code)

@michaelk83
Copy link

michaelk83 commented Jun 29, 2024

Why is KeeShare considered basic? It would argue that it's an advanced niche feature. It's not needed for most single-user use, and it requires network access.

I would also argue that Secret Service is rather basic for Linux users, as it's the main method for client applications to store and retrieve their credentials. Though I agree that it's less basic than Auto-Type and Yubikey support. And I agree that it can be grouped with SSH Agent and Browser integration, since they all add an IPC layer.

I would group these features like so:

  • Basic (no IPC and no networking): core functionality, Yubikey, Auto-Type.
  • Local IPC, no networking: above + Browser (and passkeys), SSH Agent, Secret Service.
  • Full: above + networking: Update Checks, Key Share, remote DB, icon download, HIBP.

(Technically, browser integration is also rather useless without network access, but as I understand the network access isn't done by KPXC itself.)

@varjolintu
Copy link
Member

Browser Integration still requires including networking to the code (even if it doesn't make any connections outside, it creates local sockets), so maybe it wouldn't fit to "no networking" category.

@droidmonkey droidmonkey force-pushed the refactor/simple-default-build branch from a42b85d to 304d3b7 Compare June 29, 2024 16:43
@droidmonkey
Copy link
Member Author

droidmonkey commented Jun 29, 2024

KPXC_FEATURE_NETWORK is specifically to disable TCP/IP network traffic. So if there are guards in browser using this flag then those guards need to be removed.

@droidmonkey
Copy link
Member Author

droidmonkey commented Jun 29, 2024

Why is KeeShare considered basic? It would argue that it's an advanced niche feature. It's not needed for most single-user use, and it requires network access.

It absolutely does not require network access.

KeeShare is going to be rebranded as simply "Sharing" when it gets refactored.

Don't get hung up on the Basic/Advanced wording, thought of a couple different buckets but they were all too much.

Copy link

codecov bot commented Jun 29, 2024

Codecov Report

Attention: Patch coverage is 72.50000% with 22 lines in your changes missing coverage. Please review.

Project coverage is 62.92%. Comparing base (e48ef80) to head (76624f6).
Report is 13 commits behind head on develop.

Files Patch % Lines
src/core/Tools.cpp 0.00% 5 Missing ⚠️
src/gui/reports/ReportsWidgetHibp.cpp 16.67% 5 Missing ⚠️
src/gui/DatabaseWidget.cpp 42.86% 4 Missing ⚠️
src/gui/osutils/nixutils/NixUtils.cpp 50.00% 3 Missing ⚠️
src/gui/EditWidgetIcons.cpp 60.00% 2 Missing ⚠️
src/gui/MainWindow.cpp 89.47% 2 Missing ⚠️
src/gui/reports/ReportsDialog.cpp 66.67% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #11003      +/-   ##
===========================================
- Coverage    63.05%   62.92%   -0.13%     
===========================================
  Files          362      361       -1     
  Lines        37727    37545     -182     
===========================================
- Hits         23787    23624     -163     
+ Misses       13940    13921      -19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@droidmonkey droidmonkey force-pushed the refactor/simple-default-build branch 3 times, most recently from 30d2a5d to 2569be3 Compare July 1, 2024 02:07
* make (>= 4.2) or ninja (>= 1.10)
* g++ (>= 4.9) or clang++ (>= 6.0)
* g++ (>= 9.3.0) or clang++ (>= 10.0)
* asciidoctor (>= 2.0)
Copy link
Contributor

@c4rlo c4rlo Jul 8, 2024

Choose a reason for hiding this comment

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

Could we also mention the minimum versions of required libraries (e.g. Qt, Botan) in here?

Comment on lines 54 to 57
mkdir build
cd build
cmake -DWITH_XC_ALL=ON ..
cmake ..
make
Copy link
Contributor

Choose a reason for hiding this comment

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

Or:

Suggested change
mkdir build
cd build
cmake -DWITH_XC_ALL=ON ..
cmake ..
make
cmake -B ./build
cmake --build ./build

This way it also works with alternative "generators", such as Ninja instead of Make.

INSTALL.md Show resolved Hide resolved
make
```

If you have `vcpkg` installed, add `-DCMAKE_TOOLCHAIN_FILE=${VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake` to the `cmake` command to automatically download and install all required build and runtime dependencies locally to your build directory before compiling KeePassXC. Using `vcpkg` is the preferred way to install dependencies on macOS and required on Windows if using the MSVC toolchain.

For more detailed build instructions for each platform, please refer to the [GitHub wiki](https://github.com/keepassxreboot/keepassxc/wiki/Building-KeePassXC).
Copy link
Contributor

Choose a reason for hiding this comment

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

That wiki page has a lot of info duplicated. It would be nice to standardise on a single place.

CMakeLists.txt Show resolved Hide resolved
src/autotype/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -105,10 +103,6 @@ EntryPreviewWidget::EntryPreviewWidget(QWidget* parent)
connect(m_ui->groupTabWidget, SIGNAL(tabBarClicked(int)), SLOT(updateTabIndexes()), Qt::QueuedConnection);

setFocusProxy(m_ui->entryTabWidget);

#if !defined(WITH_XC_KEESHARE)
removeTab(m_ui->groupTabWidget, m_ui->groupShareTab);
Copy link
Contributor

Choose a reason for hiding this comment

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

The removeTab function can now be removed.

droidmonkey and others added 2 commits July 28, 2024 18:27
* Remove individual feature flags in favor of a single `KPXC_MINIMAL` flag that removes advanced features from the build. Basic features are no longer guarded by feature flags.
* Basic features: Auto-Type, Yubikey, KeeShare
* Advanced features include: Browser (and passkeys), SSH Agent, and Secret Service
* Networking, Documentation, and Update Checking remain as feature flags to accommodate various distro requirements.

This change also cleans up the main CMakeLists.txt by re-arranging some content and placing macros into a dedicated include file. The minimum CMake version was bumped to 3.16.0 to conform to our minimum Ubuntu support of Focal (20.04). This also allows us to default to C++20, we fall back to C++17 for Qt versions less than 5.15.0 due to lack of support.

Lastly this change removes the KEEPASSXC_BUILD_TYPE="PreRelease" which is never used. We only support "Snapshot" and "Release" now.
@droidmonkey droidmonkey force-pushed the refactor/simple-default-build branch from e983223 to 76624f6 Compare July 28, 2024 22:27
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.

4 participants