conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.13k stars 969 forks source link

[question] Using tool-requires for inhouse-developed tools #16784

Closed fredrikkz closed 1 month ago

fredrikkz commented 1 month ago

What is your question?

Hi! We have a question regarding tool-requires, and if it's advisable to use it for inhouse developed tools.

From the docs it seems to be something we should use, as we only build an executable, and have no libs or headers we want to use:

tool_requires and tool packages are intended for executable applications, like cmake or ninja that can be used as tool_requires("cmake/[>=3.25]") by other packages to put those executables in their path. They are not intended for library-like dependencies (use requires for them), for test frameworks (use test_requires) or in general for anything that belongs to the “host” context of the final application. Do not abuse tool_requires for other purposes.

The tool we're trying to get to work (called ecsgen) is an executable that takes a number of source files, performs a transform on them, then outputs an equal amount of generated header files. We currently plan on using CMakes find_program/add_custom_command in order to trigger this executable, if this is relevant information.

To give some context about our setup, here's the output of conan graph info for our current, rather hacky, solution: image

As one can see, core, fmt, and cli11 are listed twice, both as require, but also tool-require. So, an explaination:

At first, we simply tried to add ecsgen as a tool-require for ecs_name, and added package_type = "application" to ecsgens conanfile.py. We also created a lockfile, like so:

{
    "version": "0.5",
    "requires": [
        "rg_main/1.0+editable",
        "fmt/11.0.1",
        "ecs_name/1.0+editable",
        "ecs/1.0+editable",
        "core/1.0+editable",
        "cli11/2.4.2"
    ],
    "build_requires": [
        "ecsgen/1.0+editable",
    ],
    "config_requires": []
}

However, with this lockfile, running conan graph build-order --requires=rg_main/1.0+editable ... fails with the following output:

======== Computing dependency graph ========
ERROR: Requirement 'fmt/11.0.1' not in lockfile

The error is cryptic as we can clearly see that fmt/11.0.1 is indeed listed in the lockfile, under the requires section. Our assumption of how the internal flow resolves, is that we try and resolve the tool-require for ecs_name, find ecsgen, but it has binary: Missing, so we try and resolve its dependencies. But since it's a tool-require, we aren't allowed to look at the requires section, so we fail to find the dependency.

As a test, we changed the lockfile to:

{
    "version": "0.5",
    "requires": [
        "rg_main/1.0+editable",
        "fmt/11.0.1",
        "ecsgen/1.0+editable",
        "ecs_name/1.0+editable",
        "ecs/1.0+editable",
        "core/1.0+editable",
        "cli11/2.4.2"
    ],
    "build_requires": [
        "fmt/11.0.1",
        "ecsgen/1.0+editable",
        "core/1.0+editable",
        "cli11/2.4.2"
    ],
    "config_requires": []
}

Notably, core, fmt, and cli11 are listed in build_requires as well, and ecsgen is also listed in requires. This allows conan graph build-order --requires=rg_main/1.0+editable ... to run without errors. However, the output of the command doesn't list the correct build order, as core was listed after ecsgen. That's not the end of the world, as we can quite simply analyze the output first and trigger individual builds of missing tools. Also, we needed to change to tool_requires(ecsgen, visible=True) as well, but that's expected.

Now, on to the questions:

Have you read the CONTRIBUTING guide?

fredrikkz commented 1 month ago

After pondering this for a bit, maybe the setup isn't so bad?

I guess that the build_requires actually corresponds to packages that are available for the build context, as opposed to packages in requires, that are available for the host context?

fredrikkz commented 1 month ago

Seems like the above is the correct way; it makes sense now. Sprinkling commands with --build-require as well as adding correct data in the lockfile does what it should. Closing.

memsharded commented 1 month ago

Hi @fredrikkz

Thanks for your question, and thanks specially for the details, including the graph. Sorry it took a bit to respond.

As one can see, core, fmt, and cli11 are listed twice, both as require, but also tool-require. So, an explaination:

yes, this is expected. Think in a cross build scenario, building in a Windows machine some app to run on Linux system. The fmt, cli11 of the "build" context as dependencies of escgen need to be compiled for Windows, those are linked into an app running on Windows, while the main "host" context containing the rg_main and its dependencies will be Linux binaries, and fmt and others need to be libraries built for Linux.

This build/host context also happens when not cross-building, for example when building your app for Debug, typically the "build" context of escgen will still be Release binaries.

However, with this lockfile, running conan graph build-order --requires=rg_main/1.0+editable ... fails with the following output:

When a lockfile is created, it is important that it contains all dependencies, including transitive depedencies in the "build" context. When capturing the lockfile with conan lock create it would be adviced to use the same arguments as they will be used later for example using conan lock create ... --build=missing.

Another alternative, when a lockfile is not fully complete, and not locking all necessary dependencies, is to relax it with --lockfile-partial, which means "I know there are some dependencies not locked in the lockfile, I will let them resolve dynamically.

Note that the build and host context have different sections in the lockfile, because those dependencies can in fact have different versions and revisions.

The error is cryptic as we can clearly see that fmt/11.0.1 is indeed listed in the lockfile, under the requires section.

Yes, it is complaining because it is not finding it in the "build" context. We might try to improve the error message to clarify it.

However, the output of the command doesn't list the correct build order, as core was listed after ecsgen

This shouldn't happen, the build-order must be always correct. If there is a topological incorrect error, we would need to check it. Note there are different build-orders, both recipes and configurations

Also, we needed to change to tool_requires(ecsgen, visible=True) as well, but that's expected.

This shouldn't be necessary, and could have other unintended side effects. If the build-order is not correct, then it would be better to open a new ticket for it and check it, to make sure. I think it is possible that the build order is indeed correct if it is ordering by configurations, but better check it.

Is it recommended to use tool_requires for our executable, that would generate header files? Or should we have it as a regular requires?

Yes

If we are recommended to keep it, is it actually sane to put fmt, cli11 and core as build_requires in the lockfile? Or can we somehow make conan graph build-order not fail in some other way? They will never be used as a tools-require themselves, and putting them there feels bad.

Yes, this is totally correct and expected. In the general case you shouldn't need to put them yourself manually, Conan should be able to generate a correct and complete lockfile for you.

What we're trying to achieve is a flow where an update to core and a subsequent request to build rg_main would manage to refresh all dependencies, including some tool_requires. We could of course explicitly always build all of our tools prior to building rg_main, but we feel that it should be possible somehow to get the information about what's needed from the build graph, and that would be the proper solution.

As you have already deduced, you were totally on the right track, and considering the build-host context thing, it makes more sense, your solution was correct.

Are there other parts that we are missing in our setup that would make it work better?

Check the above comments, like the conan lock create ... --build=missing, or the issue about the build-order, which should be correct.

Please don't hesitate to create new tickets for any further question, thanks very much for your feedback!

PS: A couple of side comments:

fredrikkz commented 3 weeks ago

Sorry, didn't get a notice of your reply. Here's a late one from me:

This build/host context also happens when not cross-building, for example when building your app for Debug, typically the "build" context of escgen will still be Release binaries.

That's really good feedback and not something I thought of myself - it makes total sense, thank you!

The error is cryptic as we can clearly see that fmt/11.0.1 is indeed listed in the lockfile, under the requires section.

Yes, it is complaining because it is not finding it in the "build" context. We might try to improve the error message to clarify it.

Yeah, some additional context that it was looking for a build-require would be nice

However, the output of the command doesn't list the correct build order, as core was listed after ecsgen

This shouldn't happen, the build-order must be always correct. If there is a topological incorrect error, we would need to check it. Note there are different build-orders, both recipes and configurations

I'll try and get a repro - I'm having troubles with conan graph build-order behaving in unexpected ways currently, so I can see if this still is an issue.

Also, we needed to change to tool_requires(ecsgen, visible=True) as well, but that's expected.

This shouldn't be necessary, and could have other unintended side effects. If the build-order is not correct, then it would be better to open a new ticket for it and check it, to make sure. I think it is possible that the build order is indeed correct if it is ordering by configurations, but better check it.

Will do

Check the above comments, like the conan lock create ... --build=missing, or the issue about the build-order, which should be correct.

We're trying to avoid adding it (as well as --lockfile-partial, unless building external deps). Our build servers have different build environments, so not all machines should build all deps

  • The +editable in the version is a bit unexpected, I think it wouldn't be necessary to change the version of packages put in editable mode if that is the case.

Yeah, I know, it's an experiment of mine :)

  • The html graph seems a bit outdated, we improved it a lot in recent Conan versions, what version are you using?

That would be 2.2 or 2.5 - I did an update around this time, so don't know which version it was. I'll see what 2.6 brings to the table :)

memsharded commented 3 weeks ago

Yeah, some additional context that it was looking for a build-require would be nice

Sure, makes sense, let's add it, proposing https://github.com/conan-io/conan/pull/16841

We're trying to avoid adding it (as well as --lockfile-partial, unless building external deps). Our build servers have different build environments, so not all machines should build all deps

The conan lock create --build=missing does not build missing binaries. But it is true that with Conan 2 lockfiles the --build argument in conan lock create no longer affect the outcome in most scenarios, it is just in case for some corner case that is not relevant at all for this, sorry for the noise!

That would be 2.2 or 2.5 - I did an update around this time, so don't know which version it was. I'll see what 2.6 brings to the table :)

No, probably is good, 2.5 already had the improvements, nothing new in 2.6, just your screenshot above didn't have any of the new html format sytles in https://docs.conan.io/2/reference/commands/graph/info.html#html-formatter (for missing, invalid, etc) so it looked like old style.

Thanks for all the feedback, lets continue conversation for the other topics in new tickets in case you need it, cheers!