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

[sqlite3] Control features with a configuration header #29376

Merged
merged 6 commits into from
Feb 6, 2023

Conversation

BillyONeal
Copy link
Member

rather than CMake or pkgconfig.

Resolves #29335

Alternate of #29258

@Neumann-A points out that controlling features through CMake configs and pkgconfig causes MSBuild customers to be left out in the cold. Moreover, attempting to add parenthesis to the SQLITE_API marco breaks autotools. This change makes such parenthesis unnecessary and ensures the configuration bits are used with automatic linking.

@BillyONeal BillyONeal added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Feb 2, 2023
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 6aa38234d08efefc55b70025cf6afc2212e78e15 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index c4bdb53..5617361 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -7346,7 +7346,7 @@
     },
     "sqlite3": {
       "baseline": "3.40.1",
-      "port-version": 0
+      "port-version": 1
     },
     "sqlitecpp": {
       "baseline": "3.2.0",
diff --git a/versions/s-/sqlite3.json b/versions/s-/sqlite3.json
index def50a9..eef55ad 100644
--- a/versions/s-/sqlite3.json
+++ b/versions/s-/sqlite3.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "f9e3e845c47e2aa1bd4cacced5e948e71e3409cf",
+      "version": "3.40.1",
+      "port-version": 1
+    },
     {
       "git-tree": "e906c625a802b4fb35a8ad2ff23016f76a92e7e3",
       "version": "3.40.1",

…ake or pkgconfig.

Resolves microsoft#29335

Alternate of microsoft#29258

@Neumann-A points out that controlling features through CMake configs and pkgconfig causes MSBuild customers to be left out in the cold. Moreover, attempting to add parenthesis to the SQLITE_API marco breaks autotools. This change makes such parenthesis unnecessary and ensures the configuration bits are used with automatic linking.
@BillyONeal BillyONeal force-pushed the embed-sqlite-settings branch from d9b4ebf to 348679b Compare February 2, 2023 00:31
github-actions[bot]
github-actions bot previously approved these changes Feb 2, 2023
ras0219-msft
ras0219-msft previously approved these changes Feb 2, 2023
-DPKGCONFIG_VERSION=${VERSION}
OPTIONS_DEBUG
-DSQLITE3_SKIP_TOOLS=ON
MAYBE_UNUSED_VARIABLES
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to remove this by making two separate vcpkg_check_feature() calls -- one to create the definitions for vcpkg_cmake_configure() and the other just to set the portfile variables.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

vcpkg_fixup_pkgconfig()

if(VCPKG_TARGET_IS_WINDOWS AND VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic")
vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/sqlite3.h" "# define SQLITE_API\n" "# define SQLITE_API __declspec(dllimport)\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line in sqlite3.h guarded by ifndef?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

github-actions[bot]
github-actions bot previously approved these changes Feb 2, 2023
github-actions[bot]
github-actions bot previously approved these changes Feb 2, 2023
github-actions[bot]
github-actions bot previously approved these changes Feb 2, 2023
github-actions[bot]
github-actions bot previously approved these changes Feb 3, 2023
"dependencies": [
"sqlite3"
]
"description": "Deprecated; no effects"
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I shouldn't have ever merged this back in 2020 :D #14029

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry @Hoikas

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply remove this feature?

Copy link
Contributor

Choose a reason for hiding this comment

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

vcpkg upgrade

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

is that even released yet? Even then it would probably requires 6 months of waiting before removal so that everybody can be assumed to have rebootstraped.

Copy link
Member Author

Choose a reason for hiding this comment

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

The 'needs to wait for 6 months' thing isn't a requirement for expected-to-be-only-interactive commands like upgrade. We care about install (as you saw in the default triplet change PR) because that's expected to be in build scripts where people are intentionally changing nothing about the setup.

@BillyONeal BillyONeal merged commit 185a7aa into microsoft:master Feb 6, 2023
@BillyONeal BillyONeal deleted the embed-sqlite-settings branch February 6, 2023 20:01
@dg0yt dg0yt mentioned this pull request Jul 5, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants