-
Notifications
You must be signed in to change notification settings - Fork 991
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
Bugfix libcxx detection when using CC/CXX #15418
Bugfix libcxx detection when using CC/CXX #15418
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also check broken tests
conans/client/conf/detect.py
Outdated
@@ -14,7 +14,7 @@ def detect_defaults_settings(): | |||
arch = detect_arch() | |||
if arch: | |||
result.append(("arch", arch)) | |||
compiler, version = detect_compiler() | |||
compiler, version, compiler_exe = detect_compiler() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@czoido note this is a breaking change for those that started to use the detect_api
feature, but we thought that being it declared as unstable and recently released, we are on time to change it.
The other alteranative was to opt-in via detect_compiler(return_compiler_exe=True)
or the like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that the chances to break are not so high, but my concern is that if anyone is using this, the error is going to be difficult to track, because it will look like this:
ERROR: Traceback (most recent call last):
File "/Users/test/conan/conan/cli/cli.py", line 270, in main
cli.run(args)
File "/Users/test/conan/conan/cli/cli.py", line 180, in run
command.run(self._conan_api, args[0][1:])
File "/Users/test/conan/conan/cli/command.py", line 126, in run
info = self._method(conan_api, parser, *args)
File "/Users/test/conan/conan/cli/commands/install.py", line 55, in install
profile_host, profile_build = conan_api.profiles.get_profiles_from_args(args)
File "/Users/test/conan/conan/api/subapi/profiles.py", line 66, in get_profiles_from_args
profile_host = self._get_profile(host_profiles, args.settings_host, args.options_host, args.conf_host,
File "/Users/test/conan/conan/api/subapi/profiles.py", line 88, in _get_profile
profile = loader.from_cli_args(profiles, settings, options, conf, cwd)
File "/Users/test/conan/conans/client/profile_loader.py", line 95, in from_cli_args
tmp = self.load_profile(p, cwd)
File "/Users/test/conan/conans/client/profile_loader.py", line 105, in load_profile
profile = self._load_profile(profile_name, cwd)
File "/Users/test/conan/conans/client/profile_loader.py", line 130, in _load_profile
text = rtemplate.render(context)
File "/Users/test/conan-virtual-env/lib/python3.9/site-packages/jinja2/environment.py", line 1301, in render
self.environment.handle_exception()
File "/Users/test/conan-virtual-env/lib/python3.9/site-packages/jinja2/environment.py", line 936, in handle_exception
raise rewrite_traceback_stack(source=source)
File "<template>", line 2, in top-level template code
ValueError: too many values to unpack (expected 2)
ERROR: too many values to unpack (expected 2)
If we don't have a way to raise an error that guides users on how to fix the problem, I personally would prefer a non-breaking change, like adding a new function and deprecate the current one adding a deprecation warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I agree, we can do it non-breaking and deprecating old behavior.
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
Changelog: Bugfix: Fix libcxx detection when using
CC/CXX
env vars.Docs: conan-io/docs#3509
Closes: #15399