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

fix glib compilation with msvc-clang #10029

Conversation

AndreyMlashkin
Copy link
Contributor

@AndreyMlashkin AndreyMlashkin commented Mar 30, 2022

Specify library name and version: glib/all

This is also a good place to share with all of us why you are submitting this PR (specially if it is a new addition to ConanCenter): is it a dependency of other libraries you want to package? Are you the author of the library? Thanks!


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@StellaSmith
Copy link
Contributor

I don't think this is correct, what if i want to cross-compile using clang targeting Windows?

@StellaSmith
Copy link
Contributor

StellaSmith commented Mar 30, 2022

In fact, a lot of recipes wrongly assume that you want to use clang-cl when specifying compiler=clang and os=Windows.
clang can be used standalone on windows too, not just when cross-compiling.
I'm pretty sure the correct way of using clang-cl was setting compiler=Visual Studio and compiler.toolchain=ClangCL

@StellaSmith
Copy link
Contributor

@SSE4 you seem to have created similar pull requests recently, thought you might want to know.

@SSE4
Copy link
Contributor

SSE4 commented Mar 30, 2022

@SSE4 you seem to have created similar pull requests recently, thought you might want to know.

I don't want to repeat myself, as explanation is pretty long. consider checking out #9807 (and maybe conan-io/conan#1839 as well).
there are multiple ways to use clang compiler on Windows (so far I tested at very least, 4 different ways, but there are more, actually).
unfortunately, conan model is not ideal and very limited, so it's hard to distinguish all of them. as a result, it's hard to make a recipe that satisfies all that use cases. from my point of view, it's better to support at least some of these use-cases in recipes than none of them at all.
maybe we can put these clang-cl PRs on hold, and continue discussion on how to properly model all these things in conan client itself. given 2.0 beta is going to appear in april, maybe it's finally a time to give this compiler first class support in conan. as always, suggestions are welcomed, as past discussions only led to dead end. otherwise it will be always fixiing one use-case, but breaking another use-case.

about compiler=Visual Studio + compiler.toolset=ClangCL, specifically - it has its own limitation.

  1. that's very MSBuild-specific. altough it can be supported flawlessly for MSBuild-based (and CMake-based) libraries, others (meson, autotools, custom build systems, etc.) know nothing about Visual Studio toolsets and how to invoke them.
  2. it seems to be impossible to model a clang version. toolset just says ClangCL and you have no idea if it was built with Clang 10 or Clang 13 or whatever. and if there are multiple installations in parallel - no way to specify an executable to use (as they all resolve to the same toolset, it will pick who knows which executable).
  3. again, it assumes full Visual Studio installation (or at very least, Visual Studio Build Tools). however, some users want to use clang-cl standalone in docker images (with just Windows SDK for headers/libraries). some details in issue [feature] add clang support on windows conan#9295.

@StellaSmith
Copy link
Contributor

I would prefer going with the choice of putting these clang-cl pr/issues on hold and come up with a proper solution in Conan 2.0, due to not being a real correct way of dealing with these case as of now.

I find this very ironic, thanks to clang being so flexible we don't seem to have proper a way of handling all of it's use cases.

Another thing that i still haven't seen mentioned is that clang can be used in conjunction with mingw, and the target triplet of this configuration is x86_64-windows-gnu, which differs from msys' clang x86_64-windows-msys which in turns seems to be an alias for x86_64-windows-msvc

@ericLemanissierBot
Copy link

ericLemanissierBot commented Mar 31, 2022

I detected other pull requests that are modifying glib/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@conan-center-bot

This comment has been minimized.

danimtb
danimtb previously approved these changes Apr 1, 2022
@AndreyMlashkin
Copy link
Contributor Author

I would prefer going with the choice of putting these clang-cl pr/issues on hold and come up with a proper solution in Conan 2.0, due to not being a real correct way of dealing with these case as of now.

I find this very ironic, thanks to clang being so flexible we don't seem to have proper a way of handling all of it's use cases.

Another thing that i still haven't seen mentioned is that clang can be used in conjunction with mingw, and the target triplet of this configuration is x86_64-windows-gnu, which differs from msys' clang x86_64-windows-msys which in turns seems to be an alias for x86_64-windows-msvc

is there already a concrete plan for conan 2.0?

@uilianries
Copy link
Member

@AndreyMlashkin there is a conflict on this PR. Please, fix.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@ericLemanissier ericLemanissier left a comment

Choose a reason for hiding this comment

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

this fixes some conflicts.
however, I don't think this should be merged before conan-io/conan#10955 is fixed, because "clang on windows" is not sufficient information to decide that we need to use VisualStudioBuildEnvironment

recipes/glib/all/conanfile.py Outdated Show resolved Hide resolved
recipes/glib/all/conanfile.py Outdated Show resolved Hide resolved
recipes/glib/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
@conan-center-bot

This comment has been minimized.

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
@conan-center-bot
Copy link
Collaborator

All green in build 6 (cecf5172cf03c2e0ce1d1a3fafbe87fa9ff5c598):

  • glib/2.70.4@:
    All packages built successfully! (All logs)

  • glib/2.66.2@:
    All packages built successfully! (All logs)

  • glib/2.71.2@:
    All packages built successfully! (All logs)

  • glib/2.65.3@:
    All packages built successfully! (All logs)

  • glib/2.71.3@:
    All packages built successfully! (All logs)

  • glib/2.72.0@:
    All packages built successfully! (All logs)

  • glib/2.72.1@:
    All packages built successfully! (All logs)

  • glib/2.67.6@:
    All packages built successfully! (All logs)

  • glib/2.68.3@:
    All packages built successfully! (All logs)

  • glib/2.71.1@:
    All packages built successfully! (All logs)

  • glib/2.69.3@:
    All packages built successfully! (All logs)

@ericLemanissier
Copy link
Contributor

@AndreyMlashkin please change the PR status to draft

@prince-chrismc
Copy link
Contributor

#10029 (comment)

from my point of view, it's better to support at least some of these use-cases in recipes than none of them at all.

I am in favor of a small change as long as it does not break the main workflows.

We can always circle back to make it better

@ericLemanissier
Copy link
Contributor

There is no improvement here. It breaks one configuration to fix another

@AndreyMlashkin AndreyMlashkin marked this pull request as draft May 23, 2022 07:08
@ericLemanissierBot ericLemanissierBot mentioned this pull request Jun 22, 2022
4 tasks
@stale
Copy link

stale bot commented Jul 7, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Aug 10, 2022

This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions.

@stale stale bot closed this Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants