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

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

Closed
wants to merge 6 commits into from

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented Dec 8, 2022

Changelog: (Feature): Add conan.tools.gnu.is_mingw() function
Docs: conan-io/docs#2849

Partially implement features requested in #12336 (comment). Such function is useful in recipes to know whether a user is using a canonical conan profile describing a MinGW toolchain, since there might be specific branching in a recipe based on this toolchain (using Autotools instead of NMake or MSBuild, deduce information for package info depending on logic in underlying build files etc).

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

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

@SpaceIm SpaceIm Dec 8, 2022

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)

@CLAassistant
Copy link

CLAassistant commented May 18, 2023

CLA assistant check
All committers have signed the CLA.

: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

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

@memsharded memsharded modified the milestones: 1.61, 1.62 Sep 11, 2023
@franramirez688 franramirez688 modified the milestones: 1.62, 1.63 Nov 7, 2023
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

One of the unknowns of this PR is that it introduced non-established practice in ConanCenter, in which all recipes just check for Windows&gcc, but things like cygwin or clang are not even considering.

As Conan 2.0 is already 1 year old, I am closing this PR for 1.X releases, the current workaround in recipes is enough, and I am moving it as a new ticket for 2.X releases, in order to be able to further improve recipes in the future

: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
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants