Improve database dialog behavior and Linux packaging #432
Conversation
- Enhanced database creation/registration dialog: - Option to create a directory when creating a new database - Auto-register the last accessed directory - Some security checks - Embed ICU library in global package (for GNU/Linux portability) - Remove embedded libc from opt/flamerobin/lib to prevent crashes on GNU/Linux - Update GitHub Actions workflow file: .github/workflows/linux-build.yml - Update Debian packaging files: - DEBIAN/* - deb/flamerobin/opt/flamerobin/bin/flamerobin-launcher - deb/flamerobin/opt/flamerobin/share/pixmaps/flamerobin.png
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @baldeuniversel, 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-code-assist here to provide a summary of this pull request. This PR focuses on two main areas: enhancing the user experience in the database creation/registration dialog and improving the Linux packaging for better portability and stability.
For the database dialog, the changes introduce smarter behavior in the file selection process. It now distinguishes between creating a new database (allowing file creation and prompting on overwrite) and registering an existing one (only allowing selection of existing files). It also remembers the last directory used and attempts to start the file dialog there. Additionally, basic security checks are added to ensure the selected directory exists and is writable when creating a new database.
On the Linux packaging side, the PR addresses portability issues by embedding the ICU library directly into the package. It also removes the previously embedded libc library from the installation path, which was causing crashes on some systems due to conflicts with the system's libc. The Debian packaging files have been updated to reflect these changes, including dependency adjustments, a pre-installation script to check for the required GLIBC version, and a post-installation script to clean up old libc files. The build workflow and launcher script have also been updated accordingly.
Highlights
- Database Dialog Improvements: The database creation/registration dialog's file selector now behaves differently based on whether you are creating or registering. It remembers the last used directory and includes checks to ensure the selected directory exists and is writable (for creation).
- Linux Packaging: ICU Embedding: The ICU library is now embedded within the Linux package to improve portability across different distributions.
- Linux Packaging: Libc Removal: The embedded
libclibrary is removed from the installation path during post-installation to prevent conflicts and crashes with the system'slibc. - Debian Packaging Updates: The Debian control file dependencies are adjusted, a pre-installation script checks for a minimum GLIBC version (>= 2.39), and the post-installation script handles the removal of the old embedded
libc.
Changelog
Click here to see the changelog
- src/gui/DatabaseRegistrationDialog.cpp
- Modified
OnBrowseButtonClickto handle create vs. register modes differently for file selection. - Implemented logic to load and save the last used database directory from/to configuration.
- Added checks to verify if the selected directory exists and is writable (in create mode).
- Modified
- templates/trees/deb/flamerobin/DEBIAN/control
- Updated package version from 0.9.13 to 0.9.14.
- Revised dependencies, removing several and adding
grep. - Added a minimum version requirement for
libc6(>= 2.39).
- templates/trees/deb/flamerobin/DEBIAN/postinst
- Added a step to remove any
libc.so*files from/opt/flamerobin/libafter installation. - Added a success message printed to the console.
- Added a step to remove any
- templates/trees/deb/flamerobin/DEBIAN/preinst
- Added a new pre-installation script.
- The script checks if the system's GLIBC version meets the minimum requirement (>= 2.39) before proceeding with installation.
- templates/trees/deb/flamerobin/opt/flamerobin/bin/flamerobin-launcher
- Adjusted the way
LD_LIBRARY_PATHis exported to be safer when the variable is already set. - Added
/opt/firebird/binto thePATHenvironment variable.
- Adjusted the way
- templates/trees/deb/scripts/build-icu.sh
- Added a new script to download, extract, configure, build, and install ICU version 74.2 to
/opt/icu.
- Added a new script to download, extract, configure, build, and install ICU version 74.2 to
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.
Dependencies clash,
Libc causes system crash,
Build ICU anew.
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.
Code Review
This pull request brings valuable improvements to the database registration dialog and Linux packaging. The changes to embed ICU, remove bundled libc, and add GLIBC version checks are significant steps towards better portability and stability on Linux. The dialog enhancements like remembering the last directory and performing write checks are also good user experience improvements.
I've identified a few areas for discussion and potential refinement, primarily concerning a discrepancy with the PR description for directory creation, error handling in the C++ code, and dependency declarations in the Debian control file. Addressing these will help ensure the changes are robust and align with the intended functionality.
Summary of Findings
- Discrepancy with PR Description (Directory Creation): The PR description mentions an 'Option to create a directory when creating a new database,' but the current changes in
DatabaseRegistrationDialog.cpponly check for directory existence and do not offer to create it. - Debian Package Dependencies: The
libtommath-devdependency was removed from the control file. Clarification is needed on how this dependency is now handled (e.g., static linking, no longer needed, or if a runtime version is required). - Error Handling in Configuration Access: In
DatabaseRegistrationDialog.cpp,catch (...)blocks are used to silently ignore exceptions when reading/writingLastDatabaseDirfrom/to config. It's recommended to at least log these occurrences for easier debugging. - Script Idempotency: The
build-icu.shscript's check for existing ICU sources could be improved to prevent re-downloading and re-extracting if theicu-srcdirectory (the result of a previous successful run) already exists. - File Removal Command Precision: In
postinst,rm -ris used forlibc.so*. Usingrm -fwould be more conventional for files/symlinks.
Merge Readiness
The pull request introduces several positive changes. However, due to the high-severity concern regarding the libtommath-dev dependency in the Debian control file and the medium-severity items (discrepancy with PR description on directory creation, silent error handling, script idempotency, and file removal precision), I recommend addressing these points before merging. Clarifying the dependency situation is particularly important for package stability. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from authorized maintainers after addressing the feedback.
runs standalone on a fresh GNU/Linux system (Ubuntu 24.04) : - All runtime dependencies (e.g. ICU, wxWidgets, libfbclient) are bundled with appropriate RPATH using `$ORIGIN` - Add binary/library stripping to reduce final package size Update the file -> [...DEBIAN/control] : - Remove unnecessary system-level dependencies
|
Update the workflows file -> [...linux-build.yml], now FlameRobin runs standalone on a fresh GNU/Linux system (Ubuntu 24.04) :
Update the file -> [...DEBIAN/control] :
|
…l package (relevant manager : firebird)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the database dialog's behavior and updates the Linux packaging. The changes to the dialog are a good step towards a better user experience, and the packaging updates aim for better portability.
My review highlights a few critical issues. The most significant is the new dependency on a very recent version of GLIBC (2.39) in the Debian package, which will prevent the application from installing on many common Linux distributions, including LTS versions. I've also identified a bug in the new ICU build script that causes it to re-download sources unnecessarily. In the C++ code, I've pointed out the use of empty catch blocks that can silently swallow exceptions, and I've also suggested an improvement to the database creation dialog to align it with the functionality described in the pull request. Finally, I've included a suggestion to simplify one of the post-installation shell commands.
| Architecture: all | ||
| Maintainer: Baldé <[email protected]> | ||
| Depends: bash, coreutils, tar, acl, libc6, libtommath-dev, libstdc++6, build-essential, systemd, software-properties-common, curl, bc, gzip, awk, gawk, zsh, util-linux | ||
| Depends: bash, coreutils, acl, libc6 (>= 2.39), libstdc++6, grep, bc, awk |
There was a problem hiding this comment.
The dependency on libc6 (>= 2.39) is very restrictive. GLIBC 2.39 was released in early 2024. This will prevent the package from being installed on many stable and long-term support (LTS) distributions, such as Ubuntu 22.04 (which has glibc 2.35) or Debian 12 (which has 2.36). This severely limits the portability of the package. Please consider building against an older version of GLIBC to support a wider range of systems.
| if [[ ! ( "$(printf '%s\n' "$requiredVersion" "$glibcVersion" | sort -V | head -n 1)" == "$requiredVersion" ) ]] | ||
| then | ||
|
|
||
| # | ||
| echo "~" | ||
| echo -e "\033[031;1mFlag\033[0m, required GLIBC version : GLIBC >= $requiredVersion" | ||
|
|
||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This check for GLIBC version 2.39 is too strict and will prevent installation on many widely-used Linux distributions, including recent LTS releases. For broader compatibility, it's highly recommended to build the application on an older distribution (like Ubuntu 20.04 or the oldest still-supported Debian/CentOS) to link against an older GLIBC. This would make the package usable for a much larger audience.
| } catch (...) | ||
| { | ||
| // | ||
| } |
There was a problem hiding this comment.
This catch (...) block is empty, which means any exception thrown by config().getValue() will be silently ignored. This can hide potential problems and make debugging difficult. It's better to catch specific exceptions and log them to provide visibility into potential configuration issues.
} catch (const std::exception& e)
{
wxLogWarning(wxString(_("Could not retrieve last used directory: ")) + wxString(e.what(), wxConvUTF8));
}| } catch (...) | ||
| { | ||
| // | ||
| } |
There was a problem hiding this comment.
Similar to the other try-catch block in this function, this empty catch(...) will swallow any exceptions from config().setValue(), potentially leading to silent failures where the last used directory is not saved. This should be logged to aid in debugging.
} catch (const std::exception& e)
{
wxLogWarning(wxString(_("Could not save last used directory: ")) + wxString(e.what(), wxConvUTF8));
}| INSTALL_PREFIX="/opt/icu" | ||
|
|
||
| # Download and extract ICU if not already present | ||
| if [ ! -d "icu" ]; then |
There was a problem hiding this comment.
The check if [ ! -d "icu" ] is incorrect. Inside this block, the directory icu is renamed to icu-src. On subsequent runs of the script, the icu directory will not exist, causing the script to re-download and extract the ICU sources every time, even though icu-src is already present. The check should be for the existence of icu-src.
| if [ ! -d "icu" ]; then | |
| if [ ! -d "icu-src" ]; then |
| if ( ! wxFileName::DirExists(path_dir) ) | ||
| { | ||
| wxLogError("The specified directory does not exist : %s", path_dir); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The PR description mentions an 'Option to create a directory when creating a new database'. However, the current implementation only checks if the directory exists and shows an error if it doesn't. To match the described functionality, you could prompt the user to create the directory if it's missing when in creation mode (createM).
For example:
if (!wxFileName::DirExists(path_dir)) {
if (createM) {
if (wxMessageBox(wxString::Format(_("Directory '%s' does not exist.\nDo you want to create it?"), path_dir),
_("Create directory?"), wxYES_NO | wxICON_QUESTION) == wxYES)
{
if (!wxFileName::Mkdir(path_dir, wxS_DIR_DEFAULT, wxPATH_MKDIR_FULL)) {
wxLogError(_("Failed to create directory: %s"), path_dir);
return;
}
} else {
return; // User chose not to create directory
}
} else {
wxLogError(_("The specified directory does not exist: %s"), path_dir);
return;
}
}| if ls "/opt/flamerobin/lib/libc.so*" &> /dev/null | ||
| then | ||
| rm -r /opt/flamerobin/lib/libc.so* 2> /dev/null | ||
| fi |
There was a problem hiding this comment.
This if block can be simplified. The rm command with the -f flag will not produce an error if the files do not exist, making the ls check redundant. Using rm -f is also more idiomatic for removing files than rm -r.
rm -f /opt/flamerobin/lib/libc.so* 2> /dev/null
…able sep1000 In FRInt128.cpp, sep1000 was declared as wxChar but passed to wxString::Replace() wrapped in _(). This caused a static_assert failure in wxWidgets >= 3.2 (or more) because _() only accepts string literals, not variables. The fix converts sep1000 into a wxString and removes the invalid translation macro. This ensures compatibility with wxWidgets 3.3 and preserves the intended behavior of stripping the thousands separator character.
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the database dialog and Linux packaging. My review focuses on enhancing the robustness of the new dialog logic and shell scripts. I've identified a few areas for improvement: the database dialog's error handling can be made safer by removing unnecessary try-catch blocks, and its functionality can be aligned with the PR's goals by properly handling directory creation. Additionally, I've suggested changes to the packaging scripts to make them more idiomatic and reliable. These changes should help improve the overall quality and maintainability of the code.
| try | ||
| { | ||
| config().getValue("LastDatabaseDir", lastUsedDirectory); | ||
|
|
||
| } catch (...) | ||
| { | ||
| // | ||
| } |
There was a problem hiding this comment.
This try...catch(...) block is problematic. A catch-all block like this can hide critical errors by swallowing any and all exceptions, making debugging very difficult. The config().getValue() method doesn't appear to throw exceptions; it returns a boolean indicating success. Therefore, this try...catch is likely unnecessary and should be removed. You can directly call config().getValue() and handle its return value if needed.
config().getValue("LastDatabaseDir", lastUsedDirectory);| if ( ! wxFileName::DirExists(path_dir) ) | ||
| { | ||
| wxLogError("The specified directory does not exist : %s", path_dir); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The current implementation prevents the creation of a database in a new directory by erroring out if the directory doesn't exist. This contradicts the pull request's goal to provide an "Option to create a directory when creating a new database".
When in database creation mode (createM is true), if the directory does not exist, it should be created. This can be done using wxFileName::Mkdir().
if ( ! wxFileName::DirExists(path_dir) )
{
if (createM)
{
if (!wxFileName::Mkdir(path_dir, wxS_DIR_DEFAULT, wxPATH_MKDIR_FULL))
{
wxLogError(_("Failed to create directory: %s"), path_dir);
return;
}
}
else
{
wxLogError(_("The specified directory does not exist: %s"), path_dir);
return;
}
}| try | ||
| { | ||
| config().setValue("LastDatabaseDir", path_dir); | ||
|
|
||
| } catch (...) | ||
| { | ||
| // | ||
| } |
There was a problem hiding this comment.
Similar to the earlier block, this try...catch(...) is problematic. It can hide important errors and is likely unnecessary since config().setValue() returns a boolean and is not expected to throw exceptions. It should be removed to make the code safer and cleaner.
config().setValue("LastDatabaseDir", path_dir);| // For creation mode, verify that the directory is writable | ||
| if ( createM && ! wxFileName::IsDirWritable(path_dir) ) | ||
| { | ||
| wxLogError("Write access denied : the specified directory is not writable : %s", path_dir); |
There was a problem hiding this comment.
The error message string is not marked for translation. For consistency with other user-facing strings in this file (like dialog titles), it should be wrapped in the _() macro to allow for localization.
wxLogError(_("Write access denied: the specified directory is not writable: %s"), path_dir);| if ls "/opt/flamerobin/lib/libc.so*" &> /dev/null | ||
| then | ||
| rm -r /opt/flamerobin/lib/libc.so* 2> /dev/null | ||
| fi |
There was a problem hiding this comment.
Using ls to check for file existence before removal is not ideal. A simpler and more idiomatic approach is to use rm -f, which will attempt to remove the files and won't produce an error if they don't exist. This avoids the need for the if block entirely. Also, redirecting rm's error output to /dev/null can hide important issues like permission errors.
| if ls "/opt/flamerobin/lib/libc.so*" &> /dev/null | |
| then | |
| rm -r /opt/flamerobin/lib/libc.so* 2> /dev/null | |
| fi | |
| rm -f /opt/flamerobin/lib/libc.so* |
| INSTALL_PREFIX="/opt/icu" | ||
|
|
||
| # Download and extract ICU if not already present | ||
| if [ ! -d "icu" ]; then |
There was a problem hiding this comment.
The check for the existence of the 'icu' directory is not robust and will cause the script to fail on subsequent runs. After the first successful run, the 'icu' directory is renamed to 'icu-src'. On the next run, [ ! -d "icu" ] will be true, the script will re-download the archive, and the mv icu icu-src command will fail because icu-src already exists.
The check should be for the final directory name, icu-src, to make the script idempotent.
| if [ ! -d "icu" ]; then | |
| if [ ! -d "icu-src" ]; then |
Improve database dialog behavior and Linux packaging