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

Feature: improve compiler detection #5740

Merged
merged 2 commits into from Jul 27, 2020

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Sep 12, 2019

Changelog: Feature: improve compiler detection for CONAN_V2_MODE.
Docs: conan-io/docs#1789
closes: #4322

things to test:

  • Mac apple clang
  • Mac apple clang (gcc wrapper)
  • Mac authentic clang (brew install llvm)
  • Mac authentic gcc (brew install gcc)
  • Mac LLVM-GCC (from old Xcode)
  • Linux gcc
  • Linux clang
  • Windows Visual Studio
  • Windows gcc (MinGW)
  • Windows gcc (MSYS)
  • Windows gcc (Cygwin)
  • Windows clang (clang-cl)
  • Windows clang (MSYS)
  • Windows QCC (QNX)
  • Windows icc (Intel)
  • Linux icc (Intel)
  • Mac icc (Intel)
  • ccache (compiler wrapper)
  • distcc (compiler wrapper)

what else?

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

@SSE4 SSE4 changed the title Feature: improve compiler detection WIP: Feature: improve compiler detection Sep 12, 2019
@SSE4 SSE4 force-pushed the compiler_detection branch 14 times, most recently from f655546 to 19e9511 Compare Sep 16, 2019
@SSE4 SSE4 changed the title WIP: Feature: improve compiler detection Feature: improve compiler detection Sep 16, 2019
@SSE4
Copy link
Contributor Author

SSE4 commented Sep 16, 2019

ready for the review, I have tested all configurations mentioned above

@lasote
Copy link
Contributor

lasote commented Nov 19, 2019

I'm a bit surprised that using the g++ for fixing the detection requires a new class of more than 200 lines and several changes among 6 files.
As we always say, if this is a refactor or an improvements it should be done separately.
Cannot it be just fixed without implementing new things?

@sergeyklay
Copy link

sergeyklay commented Dec 3, 2019

Any news on this?

@lasote
Copy link
Contributor

lasote commented Dec 4, 2019

@sergeyklay for me this PR is not valid for closing the issue. @SSE4 if you want to move this forward open a PR with a simpler fix and later we can discuss if we want to introduce other improvements and why.

conans/client/build/compiler_id.py Outdated Show resolved Hide resolved
conans/client/build/compiler_id.py Outdated Show resolved Hide resolved
conans/client/build/compiler_id.py Outdated Show resolved Hide resolved
conans/test/unittests/client/build/compiler_id_test.py Outdated Show resolved Hide resolved
conans/client/build/compiler_id.py Outdated Show resolved Hide resolved
conans/client/build/compiler_id.py Outdated Show resolved Hide resolved
conans/client/build/compiler_id.py Outdated Show resolved Hide resolved
conans/client/build/compiler_id.py Outdated Show resolved Hide resolved
@SSE4
Copy link
Contributor Author

SSE4 commented Mar 31, 2020

rebased, review comments addressed

Copy link
Member

@memsharded memsharded left a comment

This PR increases our test suite time by 50% in Linux, around 20% in others, depending on the run.

I am sorry, but this is not possible. Relying on building such a small app for auto-detecting compilers should probably not be the way.

Detecting the compiler versions was not a big issue so far (for the current model). We need to think if we want to follow this approach. In case we wanted, then probably:

  • Use it only for some extreme cases, when it is absolutely necessary
  • Make sure there is a fast and easy alternative for testing

So we need time to discuss this.

conans/client/build/compiler_id.py Outdated Show resolved Hide resolved
conans/client/build/compiler_id.py Outdated Show resolved Hide resolved
@SSE4
Copy link
Contributor Author

SSE4 commented Mar 31, 2020

interesting, I'll take a look at why does it slow down so much. tests I've added are mocked and shouldn't cause actual compiler commands to be run. so I am puzzled right now.

conans/client/build/compiler_id.py Outdated Show resolved Hide resolved
@memsharded
Copy link
Member

memsharded commented Jul 1, 2020

It is interesting, but I saw some differences in the CI previous job, I have relaunched in https://conan-ci.jfrog.info/blue/organizations/jenkins/ConanTestSuite/detail/PR-5740/5/pipeline/180, and still see consistently slower builds than other PR. Could be a pure CI issue, sure, but I am curious to understand why the big difference (compare for example with recent PR build: https://conan-ci.jfrog.info/blue/organizations/jenkins/ConanTestSuite/detail/PR-7284/4/pipeline/179)

@SSE4
Copy link
Contributor Author

SSE4 commented Jul 1, 2020

@memsharded build you have referenced is definitely slower for Linux disabled revisions jobs - it takes 9m instead of usual 5-8m. I'll continue to investigate why does it take slower, maybe there is still a room for improvement.

Copy link
Member

@jgsogo jgsogo left a comment

Please, if time is a concern for the test suite, let's fix it in the test suite, not in the codebase. We cannot add legacy or complexity to the codebase because of the tests. Caches are not easy to manage, I'm afraid the environment could change and the detection should run again to detect a different compiler. Can it be an issue?

If we are trying to stabilize the conan_api and document it, then we need to think about Conan being a library, the environment will change and cache won't be cleaned

If we agree with the implemented functionality (I think it is a huge improvement), then we can figure out how to minimize this extra time in the tests...

conans/client/build/compiler_id.py Outdated Show resolved Hide resolved
@SSE4 SSE4 force-pushed the compiler_detection branch 2 times, most recently from e7acdec to 35061b8 Compare Jul 11, 2020
Signed-off-by: SSE4 <tomskside@gmail.com>
@SSE4
Copy link
Contributor Author

SSE4 commented Jul 11, 2020

@jgsogo rebased, added CONAN_V2_MODE

Copy link
Member

@jgsogo jgsogo left a comment

With the v2_mode there can be no risk if want to add this feature... but we need to think how can we test this feature if this becomes the default in conan v2

conans/client/conf/detect.py Outdated Show resolved Hide resolved
conans/client/conf/detect.py Outdated Show resolved Hide resolved
@danimtb danimtb self-requested a review Jul 20, 2020
@danimtb danimtb added this to the 1.28 milestone Jul 27, 2020
@SSE4
Copy link
Contributor Author

SSE4 commented Jul 27, 2020

Q: why parsing gcc --version / gcc --dumpversion is weak?
A: there are several reasons:

QCC = "qcc"


class CompilerId(object):
Copy link
Member

@memsharded memsharded Jul 27, 2020

Choose a reason for hiding this comment

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

This could easily be a namedtuple, but it is ok, no need to change it now

@memsharded memsharded merged commit 27755da into conan-io:develop Jul 27, 2020
2 checks passed
@jgsogo
Copy link
Member

jgsogo commented Aug 3, 2020

We had some concerns about testing, any idea/plan about how to test this new detection logic if it becomes the 2.0 default?

@danimtb
Copy link
Member

danimtb commented Aug 11, 2020

I guess this is something to consider for the Conan CI itself, like having different images with environments to test compilers, build helpers, tools...

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.

[profile] Can not detect default compiler when it is c++
7 participants