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/qbs build helper #8125

Merged
merged 31 commits into from Dec 28, 2020
Merged

Feature/qbs build helper #8125

merged 31 commits into from Dec 28, 2020

Conversation

Psy-Kai
Copy link
Contributor

@Psy-Kai Psy-Kai commented Nov 30, 2020

Changelog: Feature: Add QbsToolchain and a Qbs build helper class (currently working for Mcus, not for Android or iOS).
Docs: conan-io/docs#1978

Closes #8079

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

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

This is looking good, thanks very much.
Please have a look to the comments, and also to the broken CI, as it seems the import "toolchain.qbs.generic" is wrong

from conans.errors import ConanException


class QbsException(ConanException):
Copy link
Member

Choose a reason for hiding this comment

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

Do not redefine a class with same name again, this is problematic.
Actually, it is not necessary to define a QbsException, all the other build systems do not have a custom exception for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just defined a custom exception because I wanted to prepend the origin of the exception to the message (qbs build helper and qbs toolchain). Without the custom exception I would need to add this string to every message manually. But if this is not desired I can remove the exceptions.

conans/client/build/qbs.py Outdated Show resolved Hide resolved


def _flags_from_env():
flags_from_env = {}
Copy link
Member

Choose a reason for hiding this comment

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

The way we are going to use environment variables in toolchains is still not defined, so this might be broken in the future. I am ok with adding it now this way, as long as it is clear that this integration is very experimental and subject to future breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I the environment should not be used, how do you plan to pass the compiler path or cross compiling flags? The current cross compiling goto way seems like environment variables: https://docs.conan.io/en/latest/systems_cross_building/cross_building.html#conan-v1-24-and-newer

self._conanfile = conanfile
_setup_toolchains(conanfile)
self._profile_values_from_setup = (
_read_qbs_toolchain_from_config(conanfile))
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit confused about this round-trip conversion, but probably it is because I don't know Qbs enough. If the config data is already present in a qbs format file, why is it necessary to parse it, and translate it to a conan-toolchain format one? Couldn't it be used directly from that original qbs origin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use the qbs command qbs-setup-toolchains here because it contains some logic on how to detect compilers and create a qbs-profile out of it. I did not want to mirror the logic here. The problem is that this tool creates qbs-settings which are by default user/system wide and are stored in a .ini file containing binary Qt data. So parsing this settings file cannot be easily be done by in python. This is why I use another qbs command qbs-config to read the values in a easy to parse format.

conans/__init__.py Outdated Show resolved Hide resolved
@Psy-Kai
Copy link
Contributor Author

Psy-Kai commented Nov 30, 2020

@memsharded Could it be that some tests run in an environment where CC is set?

@Psy-Kai
Copy link
Contributor Author

Psy-Kai commented Dec 10, 2020

@memsharded Could you help me here? I don't know how to fix the error in the python2.7 test.

@memsharded
Copy link
Member

Hi @Psy-Kai

Sorry for the delay. I am having a look, and doing some changes to align to latest changes in toolchains. I will push changes soon.

@memsharded
Copy link
Member

Hi @Psy-Kai

I did a few days ago a PR with a review in https://github.com/Psy-Kai/conan/pull/1. Please have a look when possible.

@Psy-Kai
Copy link
Contributor Author

Psy-Kai commented Dec 21, 2020

@memsharded There is still some problem on python2.7 regarding some *IO type class (like StringIO). I am not skilled enough to find the problem, sorry :/

@memsharded
Copy link
Member

@memsharded There is still some problem on python2.7 regarding some *IO type class (like StringIO). I am not skilled enough to find the problem, sorry :/

Yeah, don't worry, it is not evident, I am having a look, hopefully I can fix it, I will let you know.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

I think this is a good start, as a first iteration, knowing that:

  • The new toolchains and helpers are all experimental. They will change in things like folders layouts, or environment management.
  • conan.tools.xxx should only import thing from itself, things like tools.vcvars() usage will need to be refactored. Also new msvc compiler is coming soon, will need to be taken into account.
  • It seems there are some implementation details/style that could be improved (more idiomatic python), but that can be done later.
  • At some point it will be necessary to add some integration testing that actually calls qbs

I will request some other reviews too, but lets aim to merge this for Conan 1.33

@memsharded memsharded added this to the 1.33 milestone Dec 22, 2020
command_list = ['config:%s' % name]
for key, value in config_dict.items():
if type(value) is bool:
if value:
Copy link
Contributor

Choose a reason for hiding this comment

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

just FYI: shorter syntax:

b = 'true' if value else 'false'

(no need to change)

@memsharded memsharded merged commit 5260b65 into conan-io:develop Dec 28, 2020
@memsharded
Copy link
Member

Merged, this will be in Conan 1.33.

Please, try to contribute the docs and update the first comment Docs: https://github.com/conan-io/docs/pull/XXXX section, the changelog is generated automatically from this. A few hints:

  • Explicitly add that the toolchain, helper, etc, are experimental and subject to change
  • The documentation can be concise in a first iteration, and completed later, maybe just the reference section in tools.qbs.

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.

[feature] Qbs build helper
4 participants