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

[cppcommon] Fix mingw gcc build #4001

Merged

Conversation

tarc
Copy link
Contributor

@tarc tarc commented Dec 27, 2020

Specify library name and version: cppcommon/cci.20201104

  • 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.

The patch 0003 is to prevent errors related to the version of the
windows sdk made available by mingw gcc:

    error: 'SYMBOLIC_LINK_FLAG_DIRECTORY' was not declared in this scope

The patch 0004 is to prevent 'too many sessions' mingw errors.
@tarc
Copy link
Contributor Author

tarc commented Dec 27, 2020

The patch 0003 (which set _WIN32_WINNT=_WIN32_WINNT_WIN7) is to prevent errors related to the version of the windows sdk made available by mingw gcc. For instance:

C:\XXX\.conan\data\cppcommon\cci.20201104\_\_\build\c859f3ee75fa5dd94d05d4223d7f327287b27206\source_subfolder\source\filesystem\symlink.cpp:155:73: error: 'SYMBOLIC_LINK_FLAG_DIRECTORY' was not declared in this scope
         if (!CreateSymbolicLinkW(dst.wstring().c_str(), source.c_str(), SYMBOLIC_LINK_FLAG_DIRECTORY))
                                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

The patch 0004 (which pass the option -Wa,-mbig-obj for CYGWIN and MINGW builds) is to prevent 'too many sessions' errors.

It is worth noting that both this options are also set upstream by the project. The project, however, set this kind of things through cmake modules imported as git submodules from other repositories. So I decided to add them as patches to the main CMakeLists.txt instead.

@conan-center-bot
Copy link
Collaborator

Some configurations of 'cppcommon/cci.20201104' failed in build 1 (06b715376f3265220030def58c3fcae5484cb414):

@conan-center-bot
Copy link
Collaborator

All green in build 2 (3f558495466806978f6b61415b483cfc96f4e917)! 😊

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

It is worth noting that both this options are also set upstream by the project. The project, however, set this kind of things through cmake modules imported as git submodules from other repositories. So I decided to add them as patches to the main CMakeLists.txt instead.

Please add hyperlinks to the patches so there's a trace

Perhaps there's a better way

@tarc tarc mentioned this pull request Dec 27, 2020
4 tasks
@conan-center-bot
Copy link
Collaborator

All green in build 3 (2575fba18faa81bff436782db3fc23392d051536)! 😊

@conan-center-bot
Copy link
Collaborator

All green in build 4 (111a4c3efb76e61d2b94cc4e6c9c46acd9a4f688)! 😊

madebr
madebr previously approved these changes Dec 27, 2020
@tarc
Copy link
Contributor Author

tarc commented Dec 27, 2020

It is worth noting that both this options are also set upstream by the project. The project, however, set this kind of things through cmake modules imported as git submodules from other repositories. So I decided to add them as patches to the main CMakeLists.txt instead.

Please add hyperlinks to the patches so there's a trace

Perhaps there's a better way

The project specifies dependencies with the .gitlinks file:

cmake cmake https://github.com/chronoxor/CppCMakeScripts.git master

And imports those settings in CMakeLists.txt with:

# CMake module path
set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")

# Compiler features
include(SetCompilerFeatures)
include(SetCompilerWarnings)
include(SetPlatformFeatures)
include(SystemInformation)

The settings in question are provided in SetPlatformFeatures.cmake:

if(CYGWIN)

  # Base Windows platform
  add_definitions(-DWIN32)

  # Windows 10
  add_definitions(-D_WIN32_WINNT=0x0A00)

Although it sets _WIN32_WINNT to _WIN32_WINNT_WIN10. I thought there was also a setting accounting for the -Wa,-mbig-obj thing, but I couldn't find anything. I'm removing it for now...

@conan-center-bot
Copy link
Collaborator

All green in build 5 (0b94b671805b88470f9b6bf31fbd776a24d98fc2)! 😊

@prince-chrismc
Copy link
Contributor

Why are the build scripts not being pulled into CCI?

I see the author is the one who invented a tool for these .gitlinks should we not just use his tool?

@tarc
Copy link
Contributor Author

tarc commented Dec 28, 2020

Why are the build scripts not being pulled into CCI?

I see the author is the one who invented a tool for these .gitlinks should we not just use his tool?

Well, that's something that didn't occur to me XD

The author actually focuses his GIL usage on managing dependencies - I think that's why I completelly disregarded adding it, since, err, we're using Conan... But for the sake of just getting warning settings and SDK definitions, why not??

Maybe I can try to add this GIL as a build requirement?

Do you think there's a better way to approach this, @prince-chrismc?

@tarc
Copy link
Contributor Author

tarc commented Dec 28, 2020

BTW, GIL is the name of the tool that manage those .gitlinks files...

@prince-chrismc
Copy link
Contributor

But for the sake of just getting warning settings and SDK definitions, why not??

I agree, this approach is a bit move kill.

Do you think there's a better way to approach this, @prince-chrismc?

I think, #3903 is a source of inspiration.

I think the patches are good but they just need more commenting so that other mainters will be able to understand them and be able to re-apply them if needs be.

This author makes his work only usable to himself and it's a slippery slope to follow

@SSE4 SSE4 requested a review from uilianries December 29, 2020 07:55
@uilianries
Copy link
Member

uilianries commented Dec 29, 2020

I see this patch as a platform/build system patch, not an hotfix based on a bug because a feature. So I'm fine accepting this patch. Of course, it would be great adding it to the upstream.

@conan-center-bot conan-center-bot merged commit 12c4d82 into conan-io:master Dec 29, 2020
@tarc tarc deleted the feature/cppcommon_fix_mingwgcc_build branch December 31, 2020 10:23
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.

5 participants