-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[poco] RFC: Add features to POCO to allow reducing build time #43337
base: master
Are you sure you want to change the base?
Conversation
@@ -60,18 +65,16 @@ vcpkg_cmake_configure( | |||
-DPOCO_MT=${POCO_MT} | |||
-DENABLE_TESTS=OFF | |||
# Allow enabling and disabling components | |||
-DENABLE_ACTIVERECORD_COMPILER=${ENABLE_ACTIVERECORD} |
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.
This looks like it should go to vcpkg_check_features
instead, as
activerecord ENABLE_ACTIVERECORD_COMPILER
"activerecord": { | ||
"description": "ActiveRecord support for POCO" | ||
}, |
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.
Q:
Can this feature be enabled without any database backend?
i.e. What happens for
vcpkg install poco[core,activerecord]
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.
We'll need to define if we want poco[core,activerecord]
to visibly enable a data
feature, or if we're okay leaving that to POCO to resolve (which may lead to functionality being enabled but not reflected in the active vcpkg features.
POCO handles this internally by force-setting the ENABLE_DATA
option to ON
: https://github.com/pocoproject/poco/blob/705403d4f651245d39793a0a95cbf1c774c15461/CMakeLists.txt#L190-L192
Note: this isn't marked as a draft as I am hoping to get feedback on it, although I do not intend for it to be submitted as-is.
Background
We use POCO as part of a C++ plugin system, which involves building POCO into our main application/library as well as separate plugin libraries (see
Poco::ClassLoader
). I'm trying to reduce friction for new plugin authors, a large part of which is due to a slow first-compile experience which is dominated by time spent by vcpkg building POCO, especially on older/slower machines.POCO has good support for building a subset of the library, as seen in its
CMakeLists.txt
. A small subset of these options are supported by the vcpkg port today, mostly as a way to limit dependencies.Proposal
Create vcpkg features for the poco port which align with the rest of the
ENABLE_<FEATURE>
options supported by POCO'sCMakeLists.txt
, including in cases where no third-party vcpkg dependencies are required (e.g.ENABLE_MONGODB
). This would allow users to only build the relevant subset of POCO when first-build times are important.Set the default features of the poco port to be equivalent to the current functionality, to avoid breaking existing clients. Potentially remove the default features upon the next version bump, although this is not required.
I am happy to do the legwork here if the vcpkg leadership agrees with the direction, and have mocked up the changes for a few features in this PR.
History of sqlite3 feature
It seems that the vcpkg poco port has actually regressed in this area (see #42751), having recently removed a
sqlite3
feature resulting in thesqlite3
dependency being required andENABLE_DATA_SQLITE
being left in the defaultON
state. This is unfortunate, as in addition to the increased build time it also gets picked up as a dependency by open-source license and vulnerability management toolchains like ORT.Build performance impact
Removing even a few features can greatly reduce POCO's build times. I used Clang's
-ftime-trace
feature and ClangBuildAnalyzer to take some rough measurements of building the poco port locally.These numbers don't include time spent (or not) building other ports, i.e. the time saved by not building the sqlite3 port itself is not reported.
State of master as of 1/17/2025
With
activerecord
,cppparser
,pocodoc
,sqlite3
support disabledFrom the build logs I expect further savings are possible, but wanted to get an RFC up first.