-
Notifications
You must be signed in to change notification settings - Fork 1k
v3.0 upgrade sqlite3 to v3.50.0 #4972
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
base: v3.0
Are you sure you want to change the base?
Conversation
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.
Hello @mirostauder, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team,
Gemini here, providing a summary of this pull request. The primary goal of this PR is to upgrade the SQLite3 dependency used in the project to version 3.50.0. This involves updating the build process defined in the Makefile to fetch the new version and adjusting the existing custom patches to apply correctly against the updated SQLite source code. Additionally, one specific patch (sqlite3.c-multiplication-overflow.patch
) has been removed, suggesting it's either no longer necessary or incompatible with the new SQLite version.
Highlights
- Build Process Updates: The
deps/Makefile
has been modified to handle the new SQLite version, including updating patch application commands and adding a symbolic link. - Patch File Adaptations: Existing custom patches (
from_unixtime.patch
,sqlite3_pass_exts.patch
,throw.patch
) have been updated to align with the code structure of SQLite 3.50.0. - Patch Removal: The
sqlite3.c-multiplication-overflow.patch
file and its application in the Makefile have been removed.
Changelog
Click here to see the changelog
- deps/Makefile
- Updated the target directory reference for the SQLite amalgamation to 3.50.0 (line 246).
- Added a step to create a symbolic link
sqlite3
pointing to the extracted amalgamation directory (line 246). - Changed the patch application level for
from_unixtime.patch
from-p1
to-p0
(line 247). - Removed the application of the
sqlite3.c-multiplication-overflow.patch
(removed line 247).
- deps/sqlite3/from_unixtime.patch
- Updated the patch header and hunk header line numbers to correctly apply the
from_unixtime
function addition to SQLite 3.50.0 source code (lines 1-3, 48-52).
- Updated the patch header and hunk header line numbers to correctly apply the
- deps/sqlite3/sqlite3
- Updated the version string/directory reference from
sqlite-amalgamation-3430200
tosqlite-amalgamation-3500000/
(line 1).
- Updated the version string/directory reference from
- deps/sqlite3/sqlite3.c-multiplication-overflow.patch
- This file appears to have been removed, corresponding to the removal of its application in the Makefile. The patch content itself, which modified lookaside memory calculations, is no longer present.
- deps/sqlite3/sqlite3_pass_exts.patch
- Updated the patch header and hunk header line numbers to correctly apply the extension passing logic to SQLite 3.50.0 source code (lines 1-3, 187-190).
- deps/sqlite3/throw.patch
- Updated the patch header and hunk header line numbers to correctly apply the modification related to the
toupper
declaration to SQLite 3.50.0 source code (lines 1-3, 7-8).
- Updated the patch header and hunk header line numbers to correctly apply the modification related to the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A database grows,
New version, the code flows,
Patches now align.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request upgrades SQLite3 to version 3.50.0. The changes primarily involve updating the build process in the Makefile and rebasing existing patches for the new SQLite3 version. The most significant change is the removal of the sqlite3.c-multiplication-overflow.patch
. Confirmation is needed that this patch is indeed obsolete with SQLite 3.50.0. There's also a minor change in how the throw.patch
handles the toupper
declaration.
Summary of Findings
- Removal of Overflow Patch: The
sqlite3.c-multiplication-overflow.patch
has been removed. It's crucial to verify that the underlying issue is resolved in SQLite 3.50.0 to prevent reintroducing a potential vulnerability or bug. (Severity: High) - Handling of
toupper
Declaration: Thethrow.patch
now comments out theextern toupper
declaration instead of just modifying it. This change should be confirmed as intentional and correct for SQLite 3.50.0. (Severity: Medium) - Patch Rebasing: Other patches (
from_unixtime.patch
,sqlite3_pass_exts.patch
) appear to be correctly rebased for the new SQLite version, with only context line numbers changing. (Severity: Low - Not commented due to settings, informational) - Makefile Adjustments: Makefile changes for symlinking and patch application levels (
-p0
vs-p1
) seem consistent with the updated patch formats. (Severity: Low - Not commented due to settings, informational)
Merge Readiness
This pull request upgrades a critical dependency. Before merging, it's important to address the high-severity concern regarding the removal of the sqlite3.c-multiplication-overflow.patch
. Please confirm that this patch is no longer necessary with SQLite 3.50.0. The medium-severity item regarding the throw.patch
should also be clarified.
I am unable to approve pull requests directly. Please ensure these points are addressed and seek further review and approval before merging.
@@ -243,8 +243,8 @@ mariadb_client: mariadb-client-library/mariadb_client/libmariadb/libmariadbclien | |||
sqlite3/sqlite3/sqlite3.o: | |||
cd sqlite3 && rm -rf sqlite-amalgamation-*/ || true | |||
cd sqlite3 && tar -zxf sqlite-amalgamation-*.tar.gz | |||
cd sqlite3/sqlite3 && patch -p1 < ../from_unixtime.patch | |||
cd sqlite3/sqlite3 && patch sqlite3.c < ../sqlite3.c-multiplication-overflow.patch |
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 line, which applied the sqlite3.c-multiplication-overflow.patch
, has been removed. Could you please confirm that the specific multiplication overflow issue addressed by this patch (potentially related to lookaside memory allocation calculations, based on the deleted patch content) is fixed or no longer relevant in SQLite version 3.50.0? Reintroducing such an overflow could lead to correctness or stability issues.
//////////////////////////////////////////////////////////////////////////////// | ||
|
||
// ctype.h | ||
-extern int toupper (int __c) __THROW; | ||
+extern int toupper (int __c); | ||
+//extern int toupper (int __c) __THROW; |
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.
The patch for toupper
declaration has changed from removing __THROW
(i.e., extern int toupper (int __c);
) to commenting out the entire original line (//extern int toupper (int __c) __THROW;
).
Is this change to comment out the extern toupper
declaration intentional? Does sqlite3.c
version 3.50.0 now handle the toupper
declaration internally (e.g., via <ctype.h>
inclusion) making this explicit extern
declaration problematic or unnecessary?
No description provided.