Skip to content

Bump DBD::SQLite to 1.66#1174

Merged
mattias-p merged 1 commit into
zonemaster:developfrom
mattias-p:1172-large-results
Jun 28, 2024
Merged

Bump DBD::SQLite to 1.66#1174
mattias-p merged 1 commit into
zonemaster:developfrom
mattias-p:1172-large-results

Conversation

@mattias-p

@mattias-p mattias-p commented Jun 25, 2024

Copy link
Copy Markdown
Member

Purpose

This PR ensures that zm-testagent doesn't fail at runtime trying to store larger test results in the database, and that the unit tests don't fail, because of a limitation in a dependency.

Context

Fixes #1172.

On Rocky 8 and Ubuntu 20.04 the OS-provided DBD::SQLite is older than 1.66. On such systems cpanm will reject the OS package and instead pull the latest version from CPAN. The current installation instructions for Rocky and Ubuntu prescribes installing OS-packaged versions of DBD::SQLite.

We should consider what to do with the Rocky and Ubuntu installation instructions for Rocky. We have a few options, each with its own drawbacks.

  1. Leave the installation instruction untouched. Users on Rocky 8 and Ubuntu 20.04 will get two versions of DBD::SQLite but only the newer one will be used.
  2. Always install DBD::SQLite from CPAN. Quality control will be lower than needed for users on Ubuntu 22.04, 24.04 and Rocky 9.
  3. Install DBD::SQLite from CPAN on Rocky 8 and Ubuntu 20.04 and from the OS on the others. This will raise the complexity of the installation instruction yet another step.

Changes

Bumps DBD::SQLite to 1.66 in Makefile.PL.

How to test this PR

  1. Create a dist tarball including this change.
  2. Install Zonemaster Engine.
  3. Install dependencies for Zonemaster Backend.
  4. Run cpanm --test-only $DIST_TARBALL.

@mattias-p mattias-p added the V-Patch Versioning: The change gives an update of patch in version. label Jun 25, 2024
@mattias-p mattias-p added this to the v2024.1 milestone Jun 25, 2024
@mattias-p

mattias-p commented Jun 25, 2024

Copy link
Copy Markdown
Member Author

Regarding the installation instruction I have the least aversion against the drawbacks of option 1.

@matsduf

matsduf commented Jun 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

If two versions are installed then I guess it will depend on the path which one will be used. Do you see that there is a risk that the order is changed in the path leading to wrong version being used? Could we add a requirement of lowest version in the code, in this case, to avoid that wrong version is used?

All options have drawbacks, and I do not think we should use option 2. I think I prefer option 3.

@mattias-p

mattias-p commented Jun 26, 2024

Copy link
Copy Markdown
Member Author

If two versions are installed then I guess it will depend on the path which one will be used.

Yeah, the OS package manager installs packages in one place and cpanm in another.

Do you see that there is a risk that the order is changed in the path leading to wrong version being used?

The one installed by cpanm takes precedence so there should be no problem.

@matsduf

matsduf commented Jun 26, 2024

Copy link
Copy Markdown
Contributor

Do you see that there is a risk that the order is changed in the path leading to wrong version being used?

The one installed by cpanm takes precedence so there should be no problem.

The Perl path can be changed by the used, can't it? Maybe it is unlikely.

matsduf
matsduf previously approved these changes Jun 27, 2024
View reviewed changes
@matsduf matsduf linked an issue Jun 27, 2024 that may be closed by this pull request
Unit test fails on Rocky 8 #1172
Closed
@mattias-p mattias-p force-pushed the 1172-large-results branch from ab41ad3 to daf9b9d Compare June 27, 2024 14:53
@mattias-p
Make sure we can store large test results
f156bb8 
When storing large test results we construct an SQL statement with 7
variables per entry in the test result. Before DBD::SQLite 1.66 there
was a hard limit on 999 variables (SQLITE_LIMIT_VARIABLE_NUMBER). This
was only enough for small test results with up to 142 enries. In
DBD::SQLite 1.66 the limit was raised an we can store large test results
with up to 4680 entries.

N.B. This change doesn't solve the root cause. It only pushes the
problem in front of us by a good margin. IIRC the largest test result
we've observed was at about 2000 entries.
@mattias-p mattias-p force-pushed the 1172-large-results branch from daf9b9d to f156bb8 Compare June 28, 2024 08:27
@mattias-p mattias-p merged commit f538f92 into zonemaster:develop Jun 28, 2024
@mattias-p mattias-p mentioned this pull request Jul 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matsduf matsduf matsduf approved these changes
@tgreenx tgreenx Awaiting requested review from tgreenx
@marc-vanderwal marc-vanderwal Awaiting requested review from marc-vanderwal

Assignees

No one assigned

Labels

V-Patch Versioning: The change gives an update of patch in version.

Projects

None yet

Milestone

v2024.1

Development

Successfully merging this pull request may close these issues.

Unit test fails on Rocky 8

2 participants

@mattias-p @matsduf