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

Add B2Deps generator. #13592

Open
wants to merge 10 commits into
base: release/2.0
Choose a base branch
from
Open

Add B2Deps generator. #13592

wants to merge 10 commits into from

Conversation

grafikrobot
Copy link
Contributor

Changelog: Feature: Adds a B2Deps generator for the B2 build system.
Docs: conan-io/docs#3142

  • 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 release/2.0 branch, documenting this one.

conan/tools/b2/b2deps.py Outdated Show resolved Hide resolved
@grisumbras
Copy link
Contributor

grisumbras commented Apr 2, 2023

Do I understand it correctly that when this generator, for some dependencies A and B that have variation id Y, it creates a file conanbuildinfo-Y.jam? And if I then use a different profile which results in requirement of A but not B with the same variation id Y, conanbuildinfo-Y.jam will be overwritten and priviously declared alternatives for B will be lost?

If that's true, I think, the better approach would be to generate a conandep-A-Y.jam file for every dep A. This is how many CMake generators work.

Also, variation should take into account some common options, not just seetings. E.g. here's my implementation https://github.com/conan-io/conan/pull/13594/files#diff-869685af1d66e4b7f1a45a6609d7454943c8371bcbe02cda3ddc0e830cce1a23R5. Some of it was copied from the generator in Conan v1, some from Boost's Conan recipe.

@grafikrobot
Copy link
Contributor Author

Do I understand it correctly that when this generator, for some dependencies A and B that have variation id Y, it creates a file conanbuildinfo-Y.jam? And if I then use a different profile which results in requirement of A but not B with the same variation id Y, conanbuildinfo-Y.jam will be overwritten and priviously declared alternatives for B will be lost?

Ah, yes. I see the problem. Note, the problem existed in the previous b2 generator also.

If that's true, I think, the better approach would be to generate a conandep-A-Y.jam file for every dep A. This is how many CMake generators work.

Right, good idea.

Also, variation should take into account some common options, not just seetings. E.g. here's my implementation https://github.com/conan-io/conan/pull/13594/files#diff-869685af1d66e4b7f1a45a6609d7454943c8371bcbe02cda3ddc0e830cce1a23R5. Some of it was copied from the generator in Conan v1, some from Boost's Conan recipe.

Good idea. Although the only option I see in your code is shared. I do see the compiler.threads and compiler.runtime that I'm not accounting for also.

@grisumbras
Copy link
Contributor

Good idea. Although the only option I see in your code is shared. I do see the compiler.threads and compiler.runtime that I'm not accounting for also.
To be honest, the only other option I see other generators handling is fPIC.

@memsharded memsharded mentioned this pull request Apr 2, 2023
5 tasks
@memsharded
Copy link
Member

@grisumbras submitted a PR a few hours later, please read #13594 (comment)

@grafikrobot
Copy link
Contributor Author

To be honest, the only other option I see other generators handling is fPIC.

AFAIK fpic does not impact binary compatibility. So there's no point in handling it in deps. In a toolchain it would matter though.

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 wonder if given the lack of expertise in b2 by the maintainer team, if it would be better to have the generator distributed in some other way, like in ConanCenter (to be used as python_require, might require some change in ConanCenter CI to allow it). That would also allow to make the generator available simultaneously to 1.X and 2.0, and move it in parallel, taking less maintenance efforts and allowing faster fixes (not having to wait until the next Conan release)


def __init__(self, conanfile):
self._conanfile = conanfile
self._conanhome = get_conan_user_home()
Copy link
Member

Choose a reason for hiding this comment

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

Access to the get_conan_user_home() sounds like a not valid solution. A generator shouldn't need the conan home location, that should be an abstraction for the generators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used to figure out host independent paths instead of the absolute paths that normally end up in generator files. That part of the path in the generated files is replaced with a Jam variable, $(CONAN_HOME), which is dynamically obtained when the consumer does a build. I admit what I'm doing is novel. But seems perfectly doable/reasonable within the context of the generator and install IMO.

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 still not sure about this. Generators also must work the same when a package is in editable mode for example, so the package is not in the cache at all, not in the CONAN_HOME. The same should happen for "deployed" artifacts, like https://blog.conan.io/2023/05/23/Conan-agnostic-deploy-dependencies.html, where the generated files will be used without Conan, and should be relocatable.

It's used to figure out host independent paths instead of the absolute paths that normally end up in generator files.

This is another concern that should be a bit more orthogonal. I think it is better to have first the "simple" approach, in which paths are absolute paths to the cache, and work from there, based on the needs, than adding this that might have other unexpected rough edges

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points on editable and deploy. I'll have to think about those, and do some testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used to figure out host independent paths instead of the absolute paths that normally end up in generator files.

This is another concern that should be a bit more orthogonal. I think it is better to have first the "simple" approach, in which paths are absolute paths to the cache, and work from there, based on the needs, than adding this that might have other unexpected rough edges

Thing is where b2 deps puts the generated files it's expected for them to be stable and likely included in source control. As it's how the b2 conan integration works. Without that the automatic declaration of package targets just doesn't happen. Or, like the previous version of this, it means users needing to rerun the b2deps for each environment.

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 am still not sure about this. Generators also must work the same when a package is in editable mode for example, so the package is not in the cache at all, not in the CONAN_HOME. The same should happen for "deployed" artifacts, like https://blog.conan.io/2023/05/23/Conan-agnostic-deploy-dependencies.html, where the generated files will be used without Conan, and should be relocatable.

First.. There are multiple references to --deploy=full_deploy in that blog post which is incorrect. They should be --deployer=full_deploy (it confused me for a few minutes when I copy pasted commands and they errored.).

Second.. Both editable and full_deploy work just fine as is with this b2deps generator. The reason being that the use of conanhome only happens for packages that are installed to the home cache. For anything else the generated conanbuildinfo-*.jam files contain absolute paths to the packages (source path in editable or output/full_deploy). The reason being that it's a simple path replacement:

f'<search>"{b2_path(d.replace(self._conanhome, "$(CONAN_HOME)"))}"' for d in cpp_info.libdirs+cpp_info.bindirs
f' <include>"{b2_path(d.replace(self._conanhome, "$(CONAN_HOME)"))}"' for d in cpp_info.includedirs]

One thing that I could do is to make the full_deploy paths be relative to the conanbuildinfo*.jam generated files instead of absolute paths. That way they are fully relocatable. But as you said, it's better to start "simple" :-) And that's something I can work out later since I suspect full_deploy is a less common feature.

TLDR; It works as is.

Copy link
Member

@memsharded memsharded Jun 13, 2023

Choose a reason for hiding this comment

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

Thing is where b2 deps puts the generated files it's expected for them to be stable and likely included in source control.

I am afraid this is not the design of any Conan generators. They are not expected to be stable, but they are located in most cases inside the "build" folder (the generators folder inside build folder) and intended to be .gitignored and cleaned regularly. The reason for this is that these files can and will change for every single configuration change (architecture, shared, build_type), and not absolutely every binary variant can be captured in a file path of a generator.

I still think that this must be simplified and follow the rest of the generators design, allowing absolute paths in the generated files and managing them as non stable and not in version control.

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 am afraid this is not the design of any Conan generators. They are not expected to be stable, but they are located in most cases inside the "build" folder (the generators folder inside build folder) and intended to be .gitignored and cleaned regularly. The reason for this is that these files can and will change for every single configuration change (architecture, shared, build_type), and not absolutely every binary variant can be captured in a file path of a generator.

I still think that this must be simplified and follow the rest of the generators design, allowing absolute paths in the generated files and managing them as non stable and not in version control.

I guess there are a couple of different things in that..

  1. Stability of the generated files, in both names and content.
  2. Absolute or "relative" paths in the generated files.
  3. In version control.
  4. Where they are generated.

Each of those is separable from the others.

(1) I'm perfectly fine not calling them stable. After all it's just a label. They might or might not be stable.

(2) I can certainly do that. And then they are almost certainly not going to be stable.

(3) Sure, I don't have to say they can go in version control. Users might still put them there regardless though.

(4) I can't change this. If they are generated some place other than as a sibling to the conanfile.txt/py they will not. Specifically the B2 package manager integration will not load them. The declaration of the sub-targets will not work. Users would be forced to instruct/define/setup some manual way to define that "build/generator" folder to b2 instead of the automatic integration. Which goes very much against the b2 goals of removing complexity.

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 guess I could abandon the idea of the B2Deps generator entirely and create something to read the conan packages directly, and entirely in b2. Which happens to be the way a pending b2 PR to support vcpkg integration works ATM.

conan/tools/b2/b2deps.py Outdated Show resolved Hide resolved
conan/tools/b2/b2deps.py Show resolved Hide resolved
conans/test/unittests/tools/b2/test_b2deps.py Show resolved Hide resolved
@grafikrobot grafikrobot requested a review from SSE4 April 3, 2023 00:48
@grafikrobot
Copy link
Contributor Author

I wonder if given the lack of expertise in b2 by the maintainer team, if it would be better to have the generator distributed in some other way, like in ConanCenter (to be used as python_require, might require some change in ConanCenter CI to allow it). That would also allow to make the generator available simultaneously to 1.X and 2.0, and move it in parallel, taking less maintenance efforts and allowing faster fixes (not having to wait until the next Conan release)

Right.. That came up in slack here https://cpplang.slack.com/archives/C41CWV9HA/p1678955235815399. Which is why @grisumbras and I started on doing PRs. If you can work out the logistics for supporting python_requires in CCI we would love it. As that is something I've been asking for since CCI was first announced. I should note though, that not having the B2 support is currently blocking a project I want to revive and get up in CCI in a handful of weeks.

@SSE4
Copy link
Contributor

SSE4 commented Apr 3, 2023

That would also allow to make the generator available simultaneously to 1.X and 2.0, and move it in parallel, taking less maintenance efforts and allowing faster fixes (not having to wait until the next Conan release)

I don't think we should care about 1.X compatibility here, as I suppose we already agreed 1.X will not gain any new features (such as new generators), given the fact 2.0 is already released and 1.X user-base is shrinking rapidly.

@SSE4
Copy link
Contributor

SSE4 commented Apr 7, 2023

is there anything else preventing PR from merging? I would love to try it to simplify boost build.

@grafikrobot
Copy link
Contributor Author

Poking.. Is there more I should do to the PR?

@SSE4
Copy link
Contributor

SSE4 commented Apr 14, 2023

@memsharded how can we move this one forward?

@SSE4
Copy link
Contributor

SSE4 commented Apr 21, 2023

any update? if something prevents merging it, please tell, so we can address.
otherwise, if there are no objections, please merge it.

@CLAassistant
Copy link

CLAassistant commented May 18, 2023

CLA assistant check
All committers have signed the CLA.

@SSE4
Copy link
Contributor

SSE4 commented May 27, 2023

@conan-io/barbarians @jcar87 @RubenRBS @memsharded @franramirez688 is it possible to move this one forward? it's already opened for two months, I think PR is "brewed" already :) I have many people (enterprise) waiting on this one. as there were no objections for two months, I think it could be merged safely.

This makes it such that each dependency+variation pair generates an
individual conanbuildinfo-X.jam file. This avoids loosing targets when
dependencies change for the same variation.
This adds some missing features, threadapi, runtime-link,
runtime-debugging, and link properties to the variation.
To help users figure out which generated files correspond to what they
are building, and to tell which generated files they can remove, print
out in the generated files the settings and options that match each
generated file.
The generated files, and targets, used the dependency settings to
declare the b2 variants to use. That would cause problems as they don't
always match the consumer settings. This fixes that aspect and renames
the internal vars to make it clearer what's going on.
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.

None yet

5 participants