Add QbsDeps class (#10033)

[ABBAPOH]

Changelog: Feature: Add QbsDeps class to be used with Qbs Conan module provider.
Docs: Omit

This class allows Conan to export information about dependencies in the JSON format suitable for consuming by Qbs.

Instead of generating Qbs modules directly, we choose JSON format to create a clear boundary between Qbs and Conan. That way, Qbs can change the contents of the modules it create independently from Conan release schedule which provides more flexibility on Qbs side.

• 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 that there's a conflicting PR #10070. I discussed it with its author and we agreed that we should abandon that PR and go in the direction of this PR. The reason is that generating qbs modules directly from Conan doesn't have that much flexibility as in this approach, where we extract the info from Conan in a simple format (as opposed to e.g. json graph which is quite complicated).

[ABBAPOH]

I need some help with unit tests though (or maybe I should use integration tests instead?)
I copied stuff from cmake and there's a TODO that patching loops indefinitely. And indeed, attempt to use VirtualBuildEnv leads to an infinite loop.
Also, patching a class method changes all deps which prevents from constructing a proper deps tree.
I would like to test several case.

1. Simple package just with some cpp_info props but no deps whatsoever.
2. 2 packages, one depends on the other
3. Multi-component package.
   How do I do this? Cannot find an example - the only one is CMake and it looks broken for the reasons above.

[ABBAPOH]

Ok, I think I managed to fix the loop in tests, can you please verify this is sane?
Also, what's with Jenkins?

[memsharded]

Thanks very much for your contribution @ABBAPOH !

[memsharded]

I'd probably suggest an integation test instead, they are often way easier to write, they are still quite fast, and the coverage and guarantees to be correct are typically much higher. The TestClient() and other test helpers will make this also relatively straightforward to understand.

[memsharded]

I have been checking the code above and it is not clear how it will be consumed by the qbs build system.

I think that at least 1 full functional test that does a real build with dependencies (the dependency itself can be the one matrix from the fixture already existing) would be very recommended.

[ABBAPOH]

it is not clear how it will be consumed by the qbs build system.

We will pass the path to the conan build dir to qbs via CLI. This requires additional changes to Qbs in order to be done automatically, but I want to merge this first - I prefer to keep changes isolated. At least, users can pass required args manually, see examples here:
https://codereview.qt-project.org/c/qbs/qbs/+/336750/44/doc/reference/module-providers/conan-module-provider.qdoc

The proper functional test requires a new Qbs release, which is not quite there yet. I would like to add it, but there's some things I want to do first - merge qbsdeps, bring back tests for qbs toolchain and reanimate this PR https://github.com/conan-io/conan/pull/9704/commits (though I'd start from cratch probably)

[ABBAPOH]

Out of the scope of this PR but my vision is this:

1. QbsProfile class writes an ini file with qbs settings (aka profiles) to the build-dir/conan-qbs-settings/...
2. QbsDeps class writes a json files with the info about modules to build-dir/conan-qbs-deps/...
3. Qbs toolchain invokes qbs like this:
   $ qbs --settings-dir=build-dir/conan-qbs-settings moduleProviders.conan.installDir:build-dir

[ABBAPOH]

Moved to integration tests, indeed they are much simpler.

[memsharded]

Suggested change

	env_vars = VirtualBuildEnv(self._qbsdeps._conanfile).vars()
	env_vars = VirtualBuildEnv(self._qbsdeps._conanfile, , auto_generate=True).vars()

Otherwise, it disable the generation of the user side VirtualBuildEnv
This doesn't look like a QbsDeps thing, but in any case it should be part of the QbsToolchain generator, isn't it?

How is this common.json file used by Qbs?

[ABBAPOH]

We use the common.json (and environment from it) to find tools for cross-compiled builds - e.g. protobuf module for ARM contains ARM protoc, but we need to use the amd64 version which can be found in PATH in build env.
So basically we generate a "host" module for linking, but that module also embeds the info about "build" tools - that way we only need one dependency on Qbs side.

I am open for other suggestions though - the PR [0] in Qbs is not merged yet (though it covers use-cases I am aware of) so it's not too late to change the format.

[0] https://codereview.qt-project.org/c/qbs/qbs/+/336750

[ABBAPOH]

Here's what the resulting module looks like [0]. Since we embed build env in every module - maybe do the same in QbsDeps and omit common.json altogether? I kinda started with one huge file with all deps in a dict, but since I moved away from it, maybe its remnants should go as well. Leads to some duplication thought but maybe it's a minor issue.

[0] https://pastebin.com/kRQJctHp

[memsharded]

We use the common.json (and environment from it) to find tools for cross-compiled builds - e.g. protobuf module for ARM contains ARM protoc, but we need to use the amd64 version which can be found in PATH in build env.
So basically we generate a "host" module for linking, but that module also embeds the info about "build" tools - that way we only need one dependency on Qbs side.

At the moment the "build" context dependencies are not being generated. I'd probably suggest starting incrementally, supported by functional tests, the moment we add dependencies from the build context, we add the build-env, but not earlier.

[ABBAPOH]

It took me a while to understand what you mean.
Indeed, "build" deps are not generated since there was no immediate need for this. I see a couple of use-cases though and I will probably add those. However, Qbs uses the (merged) env (PATH, namely) from build deps (installed via [tool_require]) in order to locate build tools.

By design, Qbs does not distinguish between "host" and "build" modules explicitly. There might be multiple modules for different platforms/architectures within one project (meaning, more than two aka host and build). Usually, we check if arch/platform/whatever of the module match those in a Product that is currently being built. Thus, a "host" module should also contain info about build tools in order to execute those tools. That's why I insert PATH in every json and later in the module file so that when Qbs loads the "host" module, it can also locate "build" tools.

It just occurred to me that instead of using VirtualEnv and using PATH variable, I can take the location of build tools from the "build" version of the dependency and inject it in the "host" module via a variable, say "build_bindirs".
Hopefully this makes sense. Thoughts?

PS: I'll add a functional test in order to demonstrate how commands should be executed

[memsharded]

It just occurred to me that instead of using VirtualEnv and using PATH variable, I can take the location of build tools from the "build" version of the dependency and inject it in the "host" module via a variable, say "build_bindirs".
Hopefully this makes sense. Thoughts?

In general, it is much better to work and add things incrementally. Starting from the most basic use case, in this case would be just libraries in the host context via regular requires.

That allows maintainers also to get some understand and expertise about how tools such as qbs work, maybe to install it in CI, depending on the complexity. After some experience, likely improving a bit the code or some interfaces/ux, it is easier to add more advanced use cases, like injecting tools from tool_requires, environment, etc.

[memsharded]

My main concern about this PR is understanding and checking that the generated files do really work in a real project. Do you think that you can add a full functional test that does build with qbs with real dependencies? The test can be labeled with @pytest.mark.tool("qbs") to help CI pass while we install qbs there too, but still allow testing locally (with confuser_test.py definitions)

Let me know, if not I can try to contribute a test myself.

[memsharded]

The CI is broken, because the paths in windows have backslash:

_____________________________ test_empty_package ______________________________

[gw0] win32 -- Python 3.6.8 C:\J\t_v2\6152\PY36\env_7_PY36_6152\Scripts\python.exe

    def test_empty_package():
        # Checks default values generated by conan for cpp_info
        assert module_content.get('package_name') == 'mylib'
        assert module_content.get('version') == '0.1'
        assert 'package_dir' in module_content
        package_dir = module_content['package_dir']
        assert 'cpp_info' in module_content
        cpp_info = module_content['cpp_info']
>       assert cpp_info.get('includedirs', []) == [os.path.join(package_dir, 'include')]

E       AssertionError: assert ['C:\\J\\t_v2...\\p\\include'] == ['C:/J/t_v2/6...0/p\\include']
E         At index 0 diff: 'C:\\J\\t_v2\\6152\\PY36\\tmp092o89p1conans\\path with spaces\\.conan2\\p\\b\\mylibcfea037bce490\\p\\include' != 'C:/J/t_v2/6152/PY36/tmp092o89p1conans/path with spaces/.conan2/p/b/mylibcfea037bce490/p\\include'
E         Full diff:
E           [
E         -  'C:/J/t_v2/6152/PY36/tmp092o89p1conans/path with '
E         ?     ^ ^    ^    ^    ^                 ^
E         +  'C:\\J\\t_v2\\6152\\PY36\\tmp092o89p1conans\\path with '
E         ?     ^^ ^^    ^^    ^^    ^^                 ^^
E         -  'spaces/.conan2/p/b/mylibcfea037bce490/p\\include',
E         ?         ^       ^ ^ ^                  ^
E         +  'spaces\\.conan2\\p\\b\\mylibcfea037bce490\\p\\include',
E         ?         ^^       ^^ ^^ ^^                  ^^
E           ]

If you want I can try to help to fix it, I have a Windows laptop.

[ABBAPOH]

I do have Windows, it’s just I don’t have access to Jenkins to see what’s wrong.
I’ll fix this, thanks

[ABBAPOH]

Rebased on develop2

[ABBAPOH]

What's with CI? I thought test should be skipped

[memsharded]

What's with CI? I thought test should be skipped

Don't worry about that, one is a glitch and the other is Windows path backslash.

I'll check it and try to move it forward, I have assigned this PR to next Conan 2.5 release

[memsharded]

I have fixed the test and some minor other changes.

I have a concern regarding self._dep.cpp_info._package. This shouldn't be necessary, no other generator needs to access this _package private member. I have tried to change it, but some tests are breaking. Could you please have a look?

[ABBAPOH]

I have no idea how to solve this without an allowlist of properties we get from cpp_info, but maybe this is fine

[ABBAPOH]

@memsharded What's blocking the merge?

[memsharded]

@memsharded What's blocking the merge?

Nothing, sorry, just lack of time. Merging it, thanks very much for your efforts!

[memsharded]

We mark with a Docs: link to docu PR in the first comment, to connect the changelog with docs changes.

If you would like to do a PR to the docs, that would be very welcome, if not, we can manage it.
It doesn't have to be very comprenhensive, just the basics of the integration, mark things as "experimental", until things stabilize for a few releases.