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

#4265 CMake generator IS optional on Windows #4281

Merged
merged 4 commits into from Jan 17, 2019

Conversation

Projects
None yet
4 participants
@uilianries
Copy link
Member

commented Jan 11, 2019

Hi!

  • When arch is not declared and only os_build is available,
    both Linux and Macos can run CMake to generate a default
    build tool e.g. Makefiles. However, on Windows this is not
    true and raises an error.

  • This commit returns "MinGW Makefiles" for Windows when arch
    is not specified.

  • I've added unit tests to check on Windows, Linux and Macos.

Changelog: Fix: Fixes default CMake generator on Windows to use MinGW Makefiles.
Docs: conan-io/docs#1026
closes #4265

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

#4265 CMake generator IS optional on Windows
- When arch is not declared and only os_build is available,
  both Linux and Macos can run CMake to generate a default
  build tool e.g. Makefiles. However, on Windows this is not
  true and raises an error.
- This commit returns "MinGW Makefiles" for Windows when arch
  is not specified.

Signed-off-by: Uilian Ries <uilianries@gmail.com>

@ghost ghost assigned uilianries Jan 11, 2019

@ghost ghost added the stage: review label Jan 11, 2019

@SSE4

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2019

why MinGW Makefiles is default choice? (e.g. not NMake makefiles or Unix makefiles)
as I understand, it requires MinGW make, which is not something always available, right?

@uilianries

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

That's true, but if think about, both OSX Linux and MacOS have default outputs.

Also, I agree with NMake and Unix Makefiles, but our most common case on Windows is MinGW Makefiles when MSVC is not listed.

@SSE4

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

okay, need to fix the following and we're good:

======================================================================

FAIL: test_missing_settings (blocked_v2.conans.test.unittests.client.build.cmake_test.CMakeTest)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/tmp/source/341f/py27/blocked_v2/conans/test/unittests/client/build/cmake_test.py", line 1040, in test_missing_settings

    self.assertEquals(cmake.generator, None)

AssertionError: 'MinGW Makefiles' != None

uilianries added some commits Jan 14, 2019

#4625 Fix unittest for CMake generator
Signed-off-by: Uilian Ries <uilianries@gmail.com>
#4625 Fix unittest for CMake generator
Signed-off-by: Uilian Ries <uilianries@gmail.com>

@lasote lasote added this to the 1.12 milestone Jan 17, 2019

@lasote

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

Hi @uilianries! The Changelog: (Fix): Fixes #4265 is not valid, we use that line to build the changelog. So please add a description there (without the brackets in the Fix word) and move the "Fixes #..." to another line.
Docs are also required, probably it deserves to be mentioned in the CMake build helper reference. When docs are not needed you would have to specify "Docs: omit", that is also taken into account for the changelog.

@uilianries

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

@lasote Done

I didn't find any information about default generators on Docs, so I omitted.

@lasote

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

I think we could add some words at https://docs.conan.io/en/latest/reference/build_helpers/cmake.html in the generators parameter. Explaining what is the detected generator depending on the settings etc.

@uilianries

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

Agreed! I'll do it soon.

#4265 Check for CMake target generated
Signed-off-by: Uilian Ries <uilianries@gmail.com>
"dummy.cpp": CPP_CONTENT})
client.run("create . lasote/testing -s os_build={}".format(os_build))
finally:
self.assertNotIn("TypeError: argument of type 'NoneType' is not iterable", client.user_io.out)

This comment has been minimized.

Copy link
@memsharded

memsharded Jan 17, 2019

Contributor

client.out also works

In theory these checks for error are not necessary, as client.run() will raise in this cases.

@memsharded memsharded merged commit 5eb7b23 into conan-io:develop Jan 17, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.