-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
MySQL 9.1 is released #1159
MySQL 9.1 is released #1159
Conversation
WalkthroughThe pull request introduces modifications to the testing workflow and installation process for MySQL versions. The Changes
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
patches/mysql/8.0.40/skip-install-pdb.patch (3)
8-38
: Approved: Removal of .pdb file installation logicThe removal of .pdb file installation logic is appropriate given the stated reason. This change will help reduce the size of the build artifacts, which is beneficial for GitHub Actions workflows.
Consider expanding the comment slightly for better clarity:
-# .pdb files are too large to use in GitHub Actions. -# so skip installing +# .pdb files are too large to use efficiently in GitHub Actions, +# so we're skipping their installation to reduce artifact size.
41-62
: Approved: Removal of .pdb file installation for .exe and .dll filesThe removal of .pdb file installation logic for .exe and .dll files is consistent with the changes made to the INSTALL_DEBUG_SYMBOLS macro. This further contributes to reducing the size of build artifacts.
Consider adding a brief comment explaining the removal, similar to the one added in the INSTALL_DEBUG_SYMBOLS macro:
FUNCTION(INSTALL_DEBUG_TARGET target) IF(MSVC) ... ENDIF() + # .pdb file installation removed to reduce artifact size for GitHub Actions ENDFUNCTION()
1-78
: Summary: Comprehensive removal of .pdb file installationThis patch successfully removes the installation of .pdb files across different parts of the MySQL 8.0.40 build system. The changes are consistent and well-explained, contributing to the goal of reducing artifact size for GitHub Actions workflows.
Key points:
- Removed .pdb file installation from INSTALL_DEBUG_SYMBOLS macro
- Removed .pdb file installation for .exe and .dll files in INSTALL_DEBUG_TARGET function
- Removed .pdb file installation for harness plugin in Plugin.cmake
These changes will result in smaller build artifacts, which is beneficial for CI/CD processes, especially in GitHub Actions environments.
Consider documenting this change in a changelog or release notes, as it may affect developers who rely on .pdb files for debugging purposes in Windows environments.
patches/mysql/8.4.3/skip-install-pdb.patch (3)
8-37
: Approved: Removal of .pdb file installationThe removal of .pdb file installation is a good optimization for GitHub Actions environments where space is a concern. This change simplifies the installation process for Windows targets.
Consider updating the macro name to reflect its new behavior, as it no longer installs debug symbols. For example:
-MACRO(INSTALL_DEBUG_SYMBOLS target) +MACRO(SKIP_DEBUG_SYMBOLS_INSTALLATION target)This would make the macro's purpose clearer to other developers.
45-62
: Approved: Consistent removal of .pdb file installationThe removal of .pdb file installation for .exe and .dll files is consistent with the changes made in the
INSTALL_DEBUG_SYMBOLS
macro. This further simplifies the installation process and reduces the overall size of the installation.Consider adding a comment explaining why this block was removed, similar to the comment added in the
INSTALL_DEBUG_SYMBOLS
macro. For example:# .pdb files are too large to use in GitHub Actions, so skip installing
This would provide context for future developers who might wonder why this functionality was removed.
1-79
: Summary: Optimization of installation process by removing .pdb filesThis patch consistently removes the installation of .pdb files across different parts of the project (main installation, debug targets, and router component). This change:
- Reduces the overall size of the installation, which is beneficial for GitHub Actions environments.
- Simplifies the installation process for Windows targets.
- Maintains a uniform approach to handling debug symbols across the project.
These changes align well with the goal of optimizing the build and installation process for CI/CD environments.
Consider documenting this change in a changelog or release notes, as it might affect developers who rely on .pdb files for debugging. You may also want to provide alternative ways for developers to access debug symbols if needed, such as separate debug symbol packages or a symbol server.
patches/mysql/9.1.0/skip-install-pdb.patch (4)
8-38
: Approved: Removal of .pdb file installationThe removal of .pdb file installation is a reasonable optimization for GitHub Actions, as these files can be quite large. This change will help reduce build times and artifact sizes.
Consider adding a more detailed comment or documentation explaining the implications of this change, such as:
# .pdb files are too large to use in GitHub Actions. # Skipping installation to reduce build times and artifact sizes. # Note: This may impact debugging capabilities in Windows environments.
45-63
: Approved: Consistent removal of .pdb file installationThis change aligns with the earlier modification to skip .pdb file installation, ensuring consistency throughout the build process.
Consider adding a brief comment explaining why this block was removed, similar to the one added in the
INSTALL_DEBUG_SYMBOLS
macro. For example:# Removed .pdb file installation for Windows to reduce build artifact size
This will help future maintainers understand the reasoning behind this change.
70-79
: Approved: Removal of .pdb file installation for harness pluginThis change is consistent with the earlier modifications to skip .pdb file installation, ensuring that the harness plugin follows the same optimization strategy.
Consider adding a brief comment explaining the removal of this block, similar to the suggestions for the other files. For example:
# Removed .pdb file installation for harness plugin to reduce build artifact size
This will maintain consistency in documentation across the changes.
1-79
: Summary: Consistent removal of .pdb file installationThis patch successfully removes the installation of .pdb files across multiple cmake files, which will significantly reduce the size of build artifacts for GitHub Actions. The changes are consistent and well-implemented.
Consider the following recommendations:
- Add a comment in the main README or documentation explaining this change and its implications for debugging on Windows platforms.
- Ensure that there's a way to optionally generate .pdb files for local development and debugging purposes, perhaps through a cmake option.
- Update any relevant documentation or guides that may reference .pdb files or Windows debugging processes.
These steps will help maintain the project's usability across different environments while optimizing for CI/CD processes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .github/workflows/test.yml (1 hunks)
- patches/mysql/8.0.40/skip-install-pdb.patch (1 hunks)
- patches/mysql/8.4.3/skip-install-pdb.patch (1 hunks)
- patches/mysql/9.1.0/skip-install-pdb.patch (1 hunks)
- versions/mysql.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- versions/mysql.json
🧰 Additional context used
🔇 Additional comments (3)
patches/mysql/8.0.40/skip-install-pdb.patch (1)
69-78
: Approved: Removal of .pdb file installation for harness pluginThe removal of .pdb file installation for the harness plugin is consistent with the changes made in other parts of the patch. This change contributes to the overall goal of reducing artifact size and streamlining the installation process.
patches/mysql/8.4.3/skip-install-pdb.patch (1)
70-79
: Approved: Consistent removal of .pdb file installation for router componentThe removal of .pdb file installation for the router component is consistent with the changes made in other parts of the patch. This maintains a uniform approach to handling debug symbols across the project.
.github/workflows/test.yml (1)
46-46
: LGTM! Verify MySQL 9.1 availability in the action.The addition of MySQL 9.1 to the test matrix is appropriate and aligns with the PR objective. This ensures that the action is tested against the latest MySQL version.
To ensure that MySQL 9.1 is properly supported by the action, please run the following verification script:
Summary by CodeRabbit
New Features
windows-2022
,windows-2019
) and a new MySQL version (9.1
) in the testing matrix.9.1.0
and reflect recent updates to existing versions.Bug Fixes