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.

Sign up for GitHub

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

Jump to bottom

Add conan.tools.gnu.is_mingw() in conan API #12678

Closed
wants to merge 6 commits into from
Closed

Add conan.tools.gnu.is_mingw() in conan API #12678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
421d4bb
add conan.tools.gnu.is_mingw()
SpaceIm Dec 8, 2022
301e134
add unit test
SpaceIm Dec 8, 2022
e45409b
not mingw if host subsystem is cygwin or wsl
SpaceIm Dec 10, 2022
1bc05f8
clarify wsl & cygwin
SpaceIm Dec 10, 2022
44aa15c
remove wsl logic
SpaceIm Dec 28, 2022
11b3f7c
simplify
SpaceIm Dec 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions conan/tools/gnu/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from conan.tools.gnu.autotools import Autotools
from conan.tools.gnu.autotoolstoolchain import AutotoolsToolchain
from conan.tools.gnu.autotoolsdeps import AutotoolsDeps
from conan.tools.gnu.mingw import is_mingw
from conan.tools.gnu.pkgconfig import PkgConfig
from conan.tools.gnu.pkgconfigdeps import PkgConfigDeps
13 changes: 13 additions & 0 deletions conan/tools/gnu/mingw.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
def is_mingw(conanfile):
"""
Validate if current compiler in host setttings is related to MinGW
:param conanfile: ConanFile instance
:return: True, if the host compiler is related to MinGW, otherwise False.
"""
host_os = conanfile.settings.get_safe("os")
Copy link

@CJCombrink CJCombrink Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also adding my comment here[1]:

This feels like a lot of "work" that will result in false in probably most cases (I don't have numbers but I assume mingw has quite a bit lower market share compared to gcc and msvc")
I guess it would still be better to get the short circuit behaviour compared to the old code where it returns false as soon as some condition is reached that makes it false.

I would propose something like early return:

def is_mingw(conanfile):
    if conanfile.settings.get_safe("os") != "Windows":
        return False
    if conanfile.settings.get_safe("os.subsystem") == "cygwin":
        return False
    host_compiler = conanfile.settings.get_safe("compiler")
    if host_compiler == "gcc":
        return True
    if host_compiler ==  "clang" and not conanfile.settings.get_safe("compiler.runtime"):
        return True
    return False

PS: From trying to map the current code to my snippet I can only conclude that the current code is very hard to read and understand (if my mapping is not correct it proves that the proposed code is perhaps overly complex).

Copy link
Member

Choose a reason for hiding this comment

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

I agree with this, part this PR never got prioritized is because the logic reads too compact, and it also introduces new aspects that are not common practice in ConanCenter recipes, in which the majority just check Windows && gcc => MinGW

is_cygwin = host_os == "Windows" and conanfile.settings.get_safe("os.subsystem") == "cygwin"
host_compiler = conanfile.settings.get_safe("compiler")
is_mingw_gcc = host_os == "Windows" and not is_cygwin and host_compiler == "gcc"
is_mingw_clang = host_os == "Windows" and not is_cygwin and host_compiler == "clang" and \
not conanfile.settings.get_safe("compiler.runtime")
Copy link
Contributor Author

@SpaceIm SpaceIm Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

it's worth noting that I've prefered this condition over conanfile.settings.get_safe("compiler.libcxx") because libcxx is removed in configure() of pure C recipes, so it's not a reliable compiler attribute in order to disambiguate between clang mingw and llvm-clang/clang-cl.

(but honestly if there was an extra attribute in conan settings of clang compiler in order to set windows flavor, it would be far easier)

return is_mingw_gcc or is_mingw_clang
49 changes: 49 additions & 0 deletions conans/test/unittests/tools/gnu/mingw_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import pytest
from mock import Mock

from conan.tools.gnu import is_mingw
from conans import ConanFile, Settings
from conans.model.env_info import EnvValues


@pytest.mark.parametrize(
"os,compiler_name,compiler_version,compiler_libcxx,compiler_runtime,compiler_runtime_type,expected",
[
("Windows", "gcc", "9", "libstdc++11", None, None, True),
("Windows", "clang", "16", "libstdc++11", None, None, True),
("Windows", "Visual Studio", "17", "MD", None, None, False),
("Windows", "msvc", "193", None, "dynamic", "Release", False),
("Windows", "clang", "16", None, "MD", None, False),
("Windows", "clang", "16", None, "dynamic", "Release", False),
("Linux", "gcc", "9", "libstdc++11", None, None, False),
("Linux", "clang", "16", "libc++", None, None, False),
("Macos", "apple-clang", "14", "libc++", None, None, False),
],
)
def test_is_mingw(os, compiler_name, compiler_version, compiler_libcxx, compiler_runtime, compiler_runtime_type, expected):
compiler = {compiler_name: {"version": [compiler_version]}}
if compiler_libcxx:
compiler[compiler_name].update({"libcxx": [compiler_libcxx]})
if compiler_runtime:
compiler[compiler_name].update({"runtime": [compiler_runtime]})
if compiler_runtime_type:
compiler[compiler_name].update({"runtime_type": [compiler_runtime_type]})
settings = Settings({
"os": [os],
"arch": ["x86_64"],
"compiler": compiler,
"build_type": ["Release"],
})
conanfile = ConanFile(Mock(), None)
conanfile.settings = "os", "arch", "compiler", "build_type"
conanfile.initialize(settings, EnvValues())
conanfile.settings.os = os
conanfile.settings.compiler = compiler_name
conanfile.settings.compiler.version = compiler_version
if compiler_libcxx:
conanfile.settings.compiler.libcxx = compiler_libcxx
if compiler_runtime:
conanfile.settings.compiler.runtime = compiler_runtime
if compiler_runtime_type:
conanfile.settings.compiler.runtime_type = compiler_runtime_type
assert is_mingw(conanfile) == expected