search

jnsh / arc-theme

A flat theme with transparent elements (actively maintained fork)
GNU General Public License v3.0
900 stars 77 forks source link

Fix meson "sh found" spam #138

Closed nathan818fr closed 2 years ago

nathan818fr commented 3 years ago

Before: 746 occurences of "Program sh found: YES (/usr/bin/sh)" were print in the terminal during meson setup After: 1 occurence

jnsh commented 3 years ago

Indeed the build started flooding the "sh found" message obnoxiously with after some recent meson update.

The problem here is that this would bump the meson_version requirement to 0.55.0, since the output of find_program() can't be used as the script in add_install_script() before that. I'm trying to have some compatibility with older distributions (e.g. Ubuntu 20.04 only has meson 0.53.2 available), and this is probably not a good enough reason to bump the requirement.

Ideally the build system would be restructured so that there wouldn't be a need to invoke external install programs with sh. It was necessary in few spots when the old autotools based build system was supported simultaneously, and I wanted to avoid major changes to how the build worked. Now that only building with Meson is supported, the plan is to try to get rid of these eventually..

nathan818fr commented 3 years ago

I checked for backwards compatibility with custom_target(command: ) instead of add_install_script...

I would turn this pull request into a version that replaces calls to add_install_script by install_data. It is indeed a better long-term solution to use what is provided natively by meson.

nathan818fr commented 3 years ago

Um, I understand better: The result of configure_file() can be used with install_data(), but it's not possible with custom_target(). And even if custom_target() supports install/install_dir, it doesn't have a rename option like install_data()... :'( There is an open issue here, for adding an install_rename to configure_file() and install_data(): https://github.com/mesonbuild/meson/issues/7675

In the meantime, the call to sh seems to be the most interesting way to get around these restrictions.

nathan818fr commented 3 years ago

I added a meson version check to conditionally determine what to do. There is no more spam in the terminal for meson >= 0.55, otherwise the behavior remains unchanged for older versions.

jnsh commented 3 years ago

Sorry but I'm not going to accept this kind of workaround over workaround over workaround, just to fix a messy configure output. The initial workaround by simply declaring sh with find_program() would be simple enough, but adding a version check conditional to only fix the output with some meson versions gets quite ugly IMHO.

Maybe I can consider bumping the meson version requirement at some point and the the initial commit could be accepted.

The only "proper" fix would be getting rid of the sh usage altogether and the configure message flood issue isn't severe enough to justify imperfect fixes.

Um, I understand better: The result of configure_file() can be used with install_data(), but it's not possible with custom_target(). And even if custom_target() supports install/install_dir, it doesn't have a rename option like install_data()... :'(

The custom install scripts are only used in few spots where there was no way to use Meson's built in install capabilities, without restructuring the build process logic derived from the old autotools based build system. Therefore there's no drop-in solution. Getting rid of these would require restructuring of the build logic and/or file structure. It is possible that in some occasions the custom install script would still be the best/cleanest available solution.