Skip to content
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

Allow skipping the detection of specific tools in qt.has_tools() #13524

Closed
wants to merge 2 commits into from

Conversation

nirbheek
Copy link
Member

@nirbheek nirbheek commented Aug 7, 2024

This has been an annoying problem because we are unconditionally checking for lrelease in qt.has_tools(), and that program is shipped in Debian/Ubuntu with (f.ex.) qttools5-dev-tools, which pulls in the kitchen sink (100+ packages, including GStreamer).

This is really annoying when trying to do CI builds of libraries or frameworks like GStreamer that use Qt but will never ship translations.

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels generally useful I think, although I'm not sure whether it's better to take an optional list of wanted tools (defaulting to all of them) or a list of tools to skip.

mesonbuild/modules/_qt.py Outdated Show resolved Hide resolved
This has been an annoying problem because we are unconditionally
checking for `lrelease` in `qt.has_tools()`, and that program is
shipped in Debian/Ubuntu with (f.ex.) qttools5-dev-tools[1], which
pulls in the kitchen sink (100+ packages, including GStreamer).

This is really annoying when trying to do CI builds of libraries or
frameworks like GStreamer that use Qt but will never ship
translations.

1. https://packages.debian.org/sid/qttools5-dev-tools
@nirbheek nirbheek force-pushed the qt_has_tools_skip branch from 477118e to 95cdf24 Compare August 8, 2024 17:26
@nirbheek
Copy link
Member Author

nirbheek commented Aug 8, 2024

although I'm not sure whether it's better to take an optional list of wanted tools (defaulting to all of them) or a list of tools to skip.

IME lrelease is the odd-one-out. The others are shipped with the same qtbase development package on all distros. I considered making this just qt.has_tools(lrelease: bool) but we'd need new kwargs if qt adds more tools in the future, and new kwargs have to be conditional inside a version check; so are harder for projects to start using.

@chubinou
Copy link
Contributor

chubinou commented Aug 9, 2024

This feels generally useful I think, although I'm not sure whether it's better to take an optional list of wanted tools (defaulting to all of them) or a list of tools to skip.

in my #13304 PR , I proposed a patch with similar goals, I rather allowed to pass the list of tools to be check rather than opting out tools, this permit extending the tools without (I needed some QML specific tools) without having to require explicit opt-out from the user.

@nirbheek
Copy link
Member Author

I don't have a strong preference between the two, maybe @jpakkane can help us decide between this and your commit.

@nirbheek
Copy link
Member Author

In either case, the test in this PR is valuable and should be used.

@ferdnyc
Copy link
Contributor

ferdnyc commented Aug 12, 2024

There's also the possibility of the tools having other names, like lrelease-qt6 or uic-qt5. (On Fedora that's how they're named in /usr/bin to differentiate, while each /usr/lib64/qt{5,6}/bin has both names.)

IME lrelease is the odd-one-out. The others are shipped with the same qtbase development package on all distros.

"Odd one out", maybe... but, again on Fedora, the situation is sort of the opposite. lrelease is in qt6-linguist or qt5-linguist which only requires qt{5,6}-qttools-common, a 0-dependency package. It's painless to pull in.

The others are, as you say, all in qt{5,6}-qtbase-devel which is a very large collection of "dev stuff". But all necessary to do anything with Qt development.

@nirbheek
Copy link
Member Author

There's also the possibility of the tools having other names, like lrelease-qt6 or uic-qt5

That's fine, we are not talking about executable name on disk here, but about the tool's name.

@nirbheek
Copy link
Member Author

"Odd one out", maybe... but, again on Fedora, the situation is sort of the opposite

Yes, which is why I didn't notice for so long. I run Fedora. I noticed this when I tried to setup CI for building an app that uses gstreamer in an Ubuntu docker image. Installing lrelease itself ends up pulling gstreamer somehow... completely ridiculous dependency tree.

@chubinou
Copy link
Contributor

lrelease [...] It's painless to pull in.

when you build Qt yourself in a project, it's a bit annoying to be required to build tools you have no use for

IME lrelease is the odd-one-out. The others are shipped with the same qtbase development package on all distros

Issue is when you start adding support of other Qt tools, if I add support of qmlcachegen and qmltyperegistrar, this means that every people that are using has_tools will have a regression when updating meson as their code wouldn't blacklist these tools. we could restrict has_tools to only check for the current tools (moc, uic, lrelease) but then we would need another method for the new tools. By opting-in the tools you want to check, we can:

  • allow has_tool to check more tools in the future (no regression when a tool is added)
  • provide a list of default tools (so the default behavior remains the same as today)

In either case, the test in this PR is valuable and should be used.

Absolutely

@nirbheek
Copy link
Member Author

@chubinou you've convinced me. I'll move the relevant things over to your PR from here (release snippet, test) and review your PR in the next few days.

@chubinou
Copy link
Contributor

@chubinou you've convinced me. I'll move the relevant things over to your PR from here (release snippet, test) and review your PR in the next few days.

you may cherry pick my patch or rewrite it your way if you prefer, my PR may take some time to get merged

@eli-schwartz
Copy link
Member

superseded by the linked PR, which is now merged.

@nirbheek nirbheek deleted the qt_has_tools_skip branch September 30, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants