GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-45774: Autoconfiscate SQLite detection (GH-29507) by erlend-aasland · Pull Request #29507 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Nov 9, 2021 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue45774

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Erlend E. Aasland
bpo-45774: Autoconfiscate SQLite detection
f34b482
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 9, 2021
@bedevere-bot bedevere-bot added the awaiting review label Nov 9, 2021
Erlend E. Aasland added 2 commits November 9, 2021 23:30
Erlend E. Aasland
Add NEWS
8d55894
Erlend E. Aasland
Don't pollute pyconfig.h with unneeded HAVE_SQLITE_H
9bc5d7b
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 9, 2021
@bedevere-bot

bedevere-bot commented Nov 9, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 9bc5d7b 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 9, 2021
@erlend-aasland
erlend-aasland marked this pull request as ready for review November 9, 2021 23:36
@erlend-aasland
erlend-aasland requested review from ned-deily, ronaldoussoren and tiran November 9, 2021 23:36
@erlend-aasland erlend-aasland mentioned this pull request Nov 9, 2021
bpo-41930: Add support for SQLite serialise/deserialise API #26728
Merged
@erlend-aasland

erlend-aasland commented Nov 9, 2021

Copy link
Copy Markdown
Contributor Author

SQLite was not detected on buildbot AMD64 FreeBSD Shared PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
erlend-aasland marked this pull request as draft November 9, 2021 23:46
@tiran

tiran commented Nov 10, 2021

Copy link
Copy Markdown
Member

SQLite was not detected on buildbot AMD64 FreeBSD Shared PR.

@koobs where is sqlite installed on your FreeBSD buildbot? Is it in /usr/local by any chance? Would the solution https://bugs.python.org/issue45774#msg406076 work for FreeBSD?

erlend-aasland reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@koobs

koobs commented Nov 10, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

SQLite was not detected on buildbot AMD64 FreeBSD Shared PR.

@koobs where is sqlite installed on your FreeBSD buildbot? Is it in /usr/local by any chance? Would the solution https://bugs.python.org/issue45774#msg406076 work for FreeBSD?

sqlite3-3.35.5 is installed on both FreeBSD buildbot workers in LOCALBASE (/usr/local by default). This is the standard location for third party libraries installed via FreeBSD ports and official packages.

I haven't deeply reviewed the implementation in issue 45774, but I don't see a reference to --with(out)-sqlite3 to explicitly disable/enable, but otherwise and generally speaking, standard autoconf/configure checks for dependencies is fine.

I would also caution that wherever possible when doing dependency detection and configuration, we should include the ability to pass dependency specific include/library paths and *FLAGS, because Pythons build system is particularly inconsistent and flaky with respect to passing *FLAGS globally, which downstreams often have to utilise.

This can causes include ordering issues, as well as build/link time conflicts when a dependency is installed in more than one location, and both are picked up.

Other than at the configure level --with-foo-{inc,lib}=<path> (or similar), this also means Modules/Setup, if support for it is not there already (like SSL).

Further Sqlite3 Information

Installed libraries and header names:

# ls -la /usr/local/lib/libsqlite3.so*
lrwxr-xr-x  1 root  wheel       19 Oct 30 01:09 /usr/local/lib/libsqlite3.so@ -> libsqlite3.so.0.8.6
lrwxr-xr-x  1 root  wheel       19 Oct 30 01:09 /usr/local/lib/libsqlite3.so.0@ -> libsqlite3.so.0.8.6
-rwxr-xr-x  1 root  wheel  1422160 Oct 30 01:09 /usr/local/lib/libsqlite3.so.0.8.6*

# ls -la /usr/local/include/sql*
-rw-r--r--  1 root  wheel     838 Oct  4 08:15 /usr/local/include/sql3types.h
-rw-r--r--  1 root  wheel    1283 Oct  4 08:15 /usr/local/include/sqlca.h
-rw-r--r--  1 root  wheel    1586 Oct  4 08:15 /usr/local/include/sqlda-compat.h
-rw-r--r--  1 root  wheel     824 Oct  4 08:15 /usr/local/include/sqlda-native.h
-rw-r--r--  1 root  wheel     321 Oct  4 08:15 /usr/local/include/sqlda.h
-rw-r--r--  1 root  wheel  584223 Oct 30 01:09 /usr/local/include/sqlite3.h
-rw-r--r--  1 root  wheel   35437 Oct 30 01:09 /usr/local/include/sqlite3ext.h

sqlite also installed a pkg-config file:

# pkgconf --list-all |grep sql
# sqlite3                        SQLite - SQL database engine

# pkgconf sqlite3 --cflags --libs
# -I/usr/local/include -L/usr/local/lib -lsqlite3
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Nov 10, 2021

Copy link
Copy Markdown
Member

Thanks @koobs !

Yes, I agree. setup.py does some problematic things in regards of default include and linker path. We are working on a series of improvements for the build system. One goal is to move header and linker checks out of setup.py and improve Modules/Setup.local.

We don't use pkg-config for sqlite3 yet. Is /usr/local/include on the default search path?

koobs and erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@koobs

koobs commented Nov 10, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

We should assume it is (and not hardcode paths, unless there's no better alternative). Right now most if not all downstreams will be passing -isystem and/or custom build *FLAGS (along with CC et al if needed) at the global environment, and/or configure environment, and/or make environment and or Modules/Setup level to make the build work, given the current state of the Python build system. It's very tough to make things work without regressing something else. Adding another unique or special/different case would be another step backward IMO.

Sorry for the non-binary answer :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Nov 10, 2021

Copy link
Copy Markdown
Member

FYI, I summarized my plan in https://bugs.python.org/issue45573#msg406084

erlend-aasland and koobs reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Nov 11, 2021

Copy link
Copy Markdown
Contributor Author

FYI: Holding this until GH-29164 has landed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Erlend E. Aasland
Merge branch 'main' into ac-sqlite
7cd7b75
@erlend-aasland
erlend-aasland removed request for ned-deily and ronaldoussoren November 15, 2021 20:26
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 15, 2021
@bedevere-bot

bedevere-bot commented Nov 15, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 0c36ebac224953603971079cbe706a77da248d5f 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 15, 2021
Erlend E. Aasland
Prelim pkg-config support
1a33d5c
@erlend-aasland
erlend-aasland force-pushed the ac-sqlite branch from 0c36eba to 1a33d5c Compare November 16, 2021 21:38
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 16, 2021
@bedevere-bot

bedevere-bot commented Nov 18, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @tiran for commit d7c4180 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 18, 2021
@tiran

tiran commented Nov 18, 2021

Copy link
Copy Markdown
Member

Looks good! Great job! Let's do another buildbot run. I'll merge the PR tomorrow.

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Nov 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

FreeBSD still emits a warning:

checking sqlite3.h usability... yes
checking sqlite3.h presence... no
configure: WARNING: sqlite3.h: accepted by the compiler, rejected by the preprocessor!
configure: WARNING: sqlite3.h: proceeding with the compiler's result
checking for sqlite3.h... yes
checking for sqlite3_open_v2 in -lsqlite3... yes
checking for sqlite3_load_extension in -lsqlite3... yes

It also failed the configure step, but that was bco. getaddrinfo:

Fatal: You must get working getaddrinfo() function.
       or you can specify "--disable-ipv6".

UPDATE 1
How about patching CPPFLAGS in addition to instead of CFLAGS before running the AC checks?

UPDATE 2
I was previously unable to reproduce this on my FreeBSD, but that was because I had GCC installed. The FreeBSD buildbot does not have GCC installed. After pkg remove gcc, I was able to reproduce this issue, and I was also able to verify that the CPPFLAGS workaround works.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Nov 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

The ARM64 macOS buildbot failed at AC_CHECK_LIB([sqlite3], [sqlite3_open_v2], ..., leaving sqlite3 disabled. From previous runs, I see that it has SQLite 3.32.3 installed; there should be no reason for it to fail.

(The x86-64 macOS buildbot is fine, though.)

UPDATE
Suspecting the following might fix it:

diff --git a/configure.ac b/configure.ac
index 88b5a18ae9..1d87235a94 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3176,8 +3176,8 @@ AS_VAR_APPEND([LIBSQLITE3_CFLAGS], [' -I$(srcdir)/Modules/_sqlite'])
 
 AS_VAR_COPY([save_CFLAGS], [CFLAGS])
 AS_VAR_COPY([save_LDFLAGS], [LDFLAGS])
-AS_VAR_COPY([CFLAGS], [LIBSQLITE3_CFLAGS])
-AS_VAR_COPY([LDFLAGS], [LIBSQLITE3_LIBS])
+CFLAGS="$LIBSQLITE3_CFLAGS $CFLAGS"
+LDFLAGS="$LIBSQLITE3_LIBS $LDFLAGS"
 
 AC_CHECK_HEADER([sqlite3.h], [
   AC_CHECK_LIB([sqlite3], [sqlite3_open_v2], [
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Nov 19, 2021

Copy link
Copy Markdown
Member

You figured it out already!

FreeBSD has 3rd party headers in /usr/local/include. The pre-processor does not use CFLAGS. You need CPPFLAGS here. I concur that it's safe to use CPPFLAGS instead of CFLAGS for sqlite. Most pgkconf files set -I, -D, -L, and -l. Other rare cases may need both CFLAGS and CPPFLAGS, though.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Erlend E. Aasland
Address macOS (arm) and FreeBSD buildbot issues
41a78b5
@erlend-aasland

erlend-aasland commented Nov 19, 2021

Copy link
Copy Markdown
Contributor Author

If 41a78b5 looks ok for you (as a fix for both the FreeBSD and macOS arm issues), we'll run it through the buildbots again.

tiran reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran tiran added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2021
@bedevere-bot

bedevere-bot commented Nov 19, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @tiran for commit 41a78b5 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2021
@erlend-aasland

erlend-aasland commented Nov 19, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

SQLite is now detected without warnings on FreeBSD. It does however still fail bco. getaddrinfo. It also fails like that on main.

Fatal: You must get working getaddrinfo() function.
       or you can specify "--disable-ipv6".

UPDATE: This fails because LIBS is touched by AC_CHECK_LIB. After the SQLite check, LIBS contains -lsqlite3, so when we run the getaddrinfo check a little bit later, it fails because it cannot find libsqlite3. Fixed by 0173b66.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Nov 19, 2021

Copy link
Copy Markdown
Contributor Author

ARM64 macOS is also fine now. Nice.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Erlend E. Aasland
We need to save/restore CFLAGS and LIBS
0173b66
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2021
@bedevere-bot

bedevere-bot commented Nov 19, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 0173b66 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2021
tiran
tiran reviewed Nov 19, 2021
View reviewed changes

@tiran tiran left a comment

Copy link
Copy Markdown
Member

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.

Tests are passing!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction
tiran
tiran approved these changes Nov 19, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Nov 19, 2021
@erlend-aasland

erlend-aasland commented Nov 19, 2021

Copy link
Copy Markdown
Contributor Author

Tests are passing!

Finally 😆

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran tiran changed the title bpo-45774: Autoconfiscate SQLite detection bpo-45774: Autoconfiscate SQLite detection (GH-29507) Nov 19, 2021
@tiran
tiran merged commit 29e5874 into python:main Nov 19, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 19, 2021
@erlend-aasland
erlend-aasland deleted the ac-sqlite branch November 19, 2021 14:11
@koobs

koobs commented Nov 20, 2021

Copy link
Copy Markdown

Great job!

erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021
Erlend Egeberg Aasland @tiran remykarem
bpo-45774: Autoconfiscate SQLite detection (pythonGH-29507)
e4504a0
Co-authored-by: Christian Heimes <[email protected]>
@lithammer lithammer mentioned this pull request Dec 9, 2021
[email protected]: update to 3.11.0 lithammer/homebrew-deadsnakes#64
Closed
@erlend-aasland erlend-aasland mentioned this pull request May 16, 2022
Detect SQLite in configure.ac #89932
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ronaldoussoren ronaldoussoren Awaiting requested review from ronaldoussoren
@ned-deily ned-deily Awaiting requested review from ned-deily
1 more reviewer
@tiran tiran tiran approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@erlend-aasland @bedevere-bot @tiran @koobs @ned-deily @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.