-
Notifications
You must be signed in to change notification settings - Fork 988
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
Rework QbsProfile #16742
Rework QbsProfile #16742
Conversation
Tested manually on Mac, Linux, Windows using gcc, clang, msvc, clang-cl, mingw, arm-none-eabi-gcc |
I have a couple of questions though. I am not sure if the _find_exe() function is the correct way to implement this. This leads to some code duplication - e.g. exe_suffix() method in the profile and test. |
@memsharded Gentle ping |
It seems that tests are broken: The refactor of vcvars is breaking it, can you please have a look? Thanks! |
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 don't have enough knowledge to understand this PR, but the risk of adding it is zero, so I think this is good and can be moved forward, I'd just make sure that it is also labeled as "experimental" in the docs.
if the_os == 'Windows' and toolchain == 'msvc': | ||
compiler = _find_msvc(self._conanfile) | ||
elif the_os == 'Windows' and toolchain == 'clang-cl': | ||
compiler, vcvars_path = _find_clangcl(self._conanfile) |
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.
Is the standard practice in modern Qbs to have to define that full path to MSVC compiler in toolchain file for Qbs to find and use it? Or is there some default that Qbs will look for it automatically (kind of CMake), and then other cases require explicit definition (like when CMake with Ninja that can require vcvars to be activated to work)
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.
Both ways work.
We have a qbs setup-toolchains
tool that tries to detect all toolchains in a system and create a profile for each one. Than user can choose a profile which to use for their project, e.g. qbs profile:myprofile
.
If no profile is specified (or, specifically, cpp.toolchainInstallPath is not set), Qbs will try to find compiler from PATH/predefined locations.
I suppose we can omit cpp.toolchainInstallPath and tell user to run that env script that is generated by Conan, that should also work (I din't test it though).
However, since we already need to pass a bunch of properties such as defines and stuff, it seemed like a sane idea to put everything, inc toolchain path in one place, in a profile.
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.
Sounds good, thanks for the explanation.
qbs_settings_path = os.path.join(generators_folder, 'qbs_settings.txt') | ||
if not os.path.exists(qbs_settings_path): | ||
return None, None | ||
return os.path.join(generators_folder, 'conan-qbs-settings-dir'), qbs_settings_path |
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.
This settings-dir is something new, I don't fully understand the connection to the QbsProfile.
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.
Qbs settings is a collection profiles (as well which profile is default). Profile is similar to Conan profile, just a bunch of toolchain properties.
$ qbs config --list
defaultProfile: "gcc"
profiles.gcc.cpp.compilerWrapper: "/usr/bin/ccache"
profiles.gcc.cpp.toolchainInstallPath: "/usr/bin"
profiles.gcc.qbs.toolchain: "gcc"
Settings are stored in an ini file on win/linux or in a plist file on Mac (in a dir controlled by the --settings-dir flag). Because of that plist, we can't write them directly from Conan (would require a new dependency) so I write settings in an intermediate text file in this dir that is later imported using qbs config --import
and converted to an inner (ini/plist) format. I suppose I can change the inner format to JSON or use ini on all platforms in Qbs, but that won't work with old Qbs versions anyway; this hacky solution does.
Thanks very much for contributing this by the way!!! 🙂 |
By the way, we released Qbs 2.4 several weeks ago, can we install it into CI now? Do you need any help? |
The original QbsProfile class created a Qbs project file that should be imported in the project's source code. This was done that way for better compatibility with the QtCreator IDE in order for it to properly get compiler flags. This approach however, requires changes to the user project (namely, to include the generated profile file) which makes it harder to change package managers.
The new approach is to generate a qbs_settings.txt file that contains toolchain properties such as its type, install path and compiler flags and import using "qbs config --import" to the custom settings directory during resolve() stage.
Also, the old class used "qbs setup-toolchains" which main purpose is to search all toolchains in the system. This is an overkill when toolchain is known in advance since most toolchains only require 2 properties - toolchain type and its location. Thus, reimplement the required "qbs setup-toolchains" in python.
The new approach allows to test QbsProfile using integration tests which was not really possible earlier due to the dependency to the call to Qbs.
Fixes #10033
Changelog: Feature: Rework QbsProfile to support Conan 2.
Docs: conan-io/docs#3825
develop
branch, documenting this one.