-
-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
closure-size: Qt 5 and KDE 5 #12740
closure-size: Qt 5 and KDE 5 #12740
Conversation
It is not necessary to propagate the cups buildInput if Qt is configured with cups enabled.
It is not necessary to propagate the mysql buildInput if Qt is configure with MySQL enabled.
It is not necessary to propagate the postgresql buildInput if Qt is configured with PostgreSQL enabled.
It is not necessary to propagate the GTK dependencies if Qt is configured with the GTK Style enabled.
The note accompanying this dependency says "it's small and doesn't remain a runtime-dep if not used," but *neither* of those statements is true.
I have no idea how it even got here; it's certainly not necessary!
The documentation cannot be built as part of the split-module build anyway. After all the modules are built, we could build the documentation as a separate package.
The documentation cannot be built as part of the split-module build anyway. After all the modules are built, we could build the documentation as a separate package.
Otherwise, the multiple-outputs hooks will not fire correctly.
moveToOutput "include" "$dev" | ||
moveToOutput "mkspecs" "$dev" | ||
|
||
# The destination directory must exist or moveToOutput will do nothing |
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.
Oh, that should not be so. I probably have a bug in there. (Typically it should work, but perhaps it's some edge case.)
Pushed. I read mainly the cmake and qtbase changes, and they do look good. My KDE 5 packages also seem to still work fine. The total space reduction in my case is small, probably because of various leaf packages still pulling the dev stuff in. |
The docdir flag needs to include `PROJECT_NAME` according to [GNU guidelines]. We are passing `-DCMAKE_INSTALL_DOCDIR=${!outputDoc}/share/doc/${shareDocName}` but `$shareDocName` was unset. The `multiple-outputs.sh` setup hook actually only defines `shareDocName` as a local variable so it was not available for cmake setup hook. Making it global would be of limited usability, since it primarily tries to extract the project name from configure script. Additionally, it would not be set because the setup hook defines `setOutputFlags=`, preventing the function defining `shareDocName` from running. And lastly, the function would not run for single-output derivations. Previously, we tried [not disabling `setOutputFlags`] and passing the directory flags only for multi-output derivations that do not disable `setOutputFlags` but that meant having two different branches of code, making it harder to check correctness. The multi-output one did in fact not work due to aforementioned undefined `shareDocName`. It also broke derivations that set `setOutputFlags=` like [`qtModule` function does] (probably because some Qt modules have configure scripts incompatible with `configureFlags` defined by `multiple-outputs.sh` setup hook). For that reason, it was [reverted], putting us back to start. Let’s try to extract the project name from CMake in the cmake setup hook. CMake has a `-L` flag for dumping variables but `PROJECT_NAME` did not seem to be among them when I tested, so I had to resort to parsing the `CMakeLists.txt` file. The extraction function is limited, it does not deal with * project name on different line from the `project(` command opening - that will just not get matched so we will fall back to using the derivation name * variable interpolation - we will just fall back to using derivation name when the extracted `project_name` contains a dollar character * multiple [`project`] commands - The command sets `PROJECT_NAME` variable anew with each call, so the last `project` call before `include(GNUInstallDirs)` command will be used when the included module would [cache the `CMAKE_INSTALL_DOCDIR` variable]. We will just take the first discovered `project` command for simplicity. Hopefully, there are not many projects that use multiple `project` calls before including `GNUInstallDirs`. In either case, we will have some subdirectory so the conflicts will be minimized. [GNU guidelines]: https://www.gnu.org/prep/standards/html_node/Directory-Variables.html#index-docdir [not disabling `setOutputFlags`]: NixOS@be1b225 [`qtModule` function does]: NixOS#12740 [reverted]: NixOS#92298 [`PROJECT_NAME`]: https://cmake.org/cmake/help/v3.18/variable/PROJECT_NAME.html [`project`]: https://cmake.org/cmake/help/v3.18/command/project.html [cache the `CMAKE_INSTALL_DOCDIR` variable]: https://github.com/Kitware/CMake/blob/92e30d576d66ac05254bba0f0ff7d655947beb0f/Modules/GNUInstallDirs.cmake#L298-L299
The docdir flag needs to include `PROJECT_NAME` according to [GNU guidelines]. We are passing `-DCMAKE_INSTALL_DOCDIR=${!outputDoc}/share/doc/${shareDocName}` but `$shareDocName` was unset. The `multiple-outputs.sh` setup hook actually only defines `shareDocName` as a local variable so it was not available for cmake setup hook. Making it global would be of limited usability, since it primarily tries to extract the project name from configure script. Additionally, it would not be set because the setup hook defines `setOutputFlags=`, preventing the function defining `shareDocName` from running. And lastly, the function would not run for single-output derivations. Previously, we tried [not disabling `setOutputFlags`] and passing the directory flags only for multi-output derivations that do not disable `setOutputFlags` but that meant having two different branches of code, making it harder to check correctness. The multi-output one did in fact not work due to aforementioned undefined `shareDocName`. It also broke derivations that set `setOutputFlags=` like [`qtModule` function does] (probably because some Qt modules have configure scripts incompatible with `configureFlags` defined by `multiple-outputs.sh` setup hook). For that reason, it was [reverted], putting us back to start. Let’s try to extract the project name from CMake in the cmake setup hook. CMake has a `-L` flag for dumping variables but `PROJECT_NAME` did not seem to be among them when I tested, so I had to resort to parsing the `CMakeLists.txt` file. The extraction function is limited, it does not deal with * project name on different line from the `project(` command opening - that will just not get matched so we will fall back to using the derivation name * variable interpolation - we will just fall back to using derivation name when the extracted `project_name` contains a dollar character * multiple [`project`] commands - The command sets `PROJECT_NAME` variable anew with each call, so the last `project` call before `include(GNUInstallDirs)` command will be used when the included module would [cache the `CMAKE_INSTALL_DOCDIR` variable]. We will just take the first discovered `project` command for simplicity. Hopefully, there are not many projects that use multiple `project` calls before including `GNUInstallDirs`. In either case, we will have some subdirectory so the conflicts will be minimized. [GNU guidelines]: https://www.gnu.org/prep/standards/html_node/Directory-Variables.html#index-docdir [not disabling `setOutputFlags`]: NixOS@be1b225 [`qtModule` function does]: NixOS#12740 [reverted]: NixOS#92298 [`PROJECT_NAME`]: https://cmake.org/cmake/help/v3.18/variable/PROJECT_NAME.html [`project`]: https://cmake.org/cmake/help/v3.18/command/project.html [cache the `CMAKE_INSTALL_DOCDIR` variable]: https://github.com/Kitware/CMake/blob/92e30d576d66ac05254bba0f0ff7d655947beb0f/Modules/GNUInstallDirs.cmake#L298-L299
I'm making this a pull request against the closure-size branch so I can get some code review before merging it; it's a very big patch set. All the Qt 5 packages take advantage of closure size reductions now, to great effect. I wanted to get all those changes done because they cause mass rebuilds. I also fixed CMake with respect to the multiple outputs feature. Some of the KDE packages may not be taking best advantage multiple outputs yet, especially ones that still use Qt 4, but making changes to "leaf" packages in the dependency tree later isn't a big deal. Mostly, I just want to get this merged before there are more breaking changes! 😄
Testing performed:
nox-review
and running my system from the reduced-closure Plasma desktop./cc @vcunat