Skip to content

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Dec 30, 2024

Closes #449

@zanieb zanieb added platform:darwin Specific to the macOS platform platform:linux Specific to the Linux platform labels Dec 30, 2024
@zanieb
Copy link
Member Author

zanieb commented Dec 30, 2024

I need to validate this works on other Python versions too, I only tested 3.14 in #457.

A bit separately, I'm not sure how the SQLite checks passed without the patch at python/cpython#128322 since that was required in a test outside of python-build-standalone (as reported in the issue linked there).

Copy link
Collaborator

@indygreg indygreg left a comment

Choose a reason for hiding this comment

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

This looks correct. I'm surprised a non-working pkg-config survived this long in the project's history!

Although TBH I'm not sure how much it mattered before the C extensions stuff was ported to configure. And even post configure, our approach has historically been to spit out a Modules file explicitly defining how to compile extension modules, bypassing the CPython build system nearly entirely.

@indygreg
Copy link
Collaborator

Running just diff-python-json against this branch yields the following changes on a 3.13 Linux build:

  • HAVE_EDITLINE_READLINE_H 1 -> 0
  • HAVE_LIBSQLITE3 0 -> 1
  • HAVE_LZMA_H 1 -> 0
  • HAVE_RL_APPEND_HISTORY 0 -> 1
  • HAVE_RL_CATCH_SIGNAL 0 -> 1
  • HAVE_RL_COMPLETION_APPEND_CHARACTER 0 -> 1
  • HAVE_RL_COMPLETION_DISPLAY_MATCHES_HOOK 0 -> 1
  • HAVE_RL_COMPLETION_MATCHES 0 -> 1
  • HAVE_RL_COMPLETION_SUPPRESS_APPEND 0 -> 1
  • HAVE_RL_PRE_INPUT_HOOK 0 -> 1
  • HAVE_RL_RESIZE_TERMINAL 0 -> 1
  • PY_SQLITE_HAVE_SERIALIZE 0 -> 1
  • Py_RL_STARTUP_HOOK_TAKES_ARGS 0 -> 1
  • WITH_EDITLINE 0 -> 1

So it looks like some readline/libedit config changes snuck in here as well. I want to say the new state is completely correct.

The only concerning change here is HAVE_LZMA_H going to off. Maybe the configure script was pulling in the Docker image's libxz via its pkg-config? As long as we build a functional xz extension module, I think we're good here though.

@indygreg
Copy link
Collaborator

And if we run a full just diff, we see changes in the following object files:

  • python/build/Modules/_sqlite/connection.o
  • python/build/Modules/readline.o

No changes to anything libxz related, which is good.

If you look at the raw changes to the ELF binaries, it looks like a handful of new symbol references are introduced, which means Python is taking advantage of features that weren't detected/enabled before.

So this all looks good to me.

(It might be a good idea to have CI run diffoscope against the base branch and post a summary to the PR to make this kind of analysis more turnkey.)

@zanieb
Copy link
Member Author

zanieb commented Dec 30, 2024

Thanks for doing those comparisons Greg! I was thinking it'd be wise to check what else changed but wasn't sure what the best way to do so is yet. I was going to diff the logs as a starting point 😭

(It might be a good idea to have CI run diffoscope against the base branch and post a summary to the PR to make this kind of analysis more turnkey.)

That'd be sweet. I'll look into that.

@zanieb zanieb merged commit 4c4d347 into main Jan 1, 2025
297 checks passed
@zanieb zanieb deleted the zb/sqlite-fix branch January 1, 2025 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:darwin Specific to the macOS platform platform:linux Specific to the Linux platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

python-build-standalone ships sqlite3 module missing Connection.serialize, deserialize
3 participants