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

[question] Using tool_requires dependencies in source method #15152

Closed
1 task done
cirrus-haven-trimble opened this issue Nov 21, 2023 · 8 comments · Fixed by #15153
Closed
1 task done

[question] Using tool_requires dependencies in source method #15152

cirrus-haven-trimble opened this issue Nov 21, 2023 · 8 comments · Fixed by #15153

Comments

@cirrus-haven-trimble
Copy link

What is your question?

I maintain a number of Conan packages for internal use by our team, and have recently been working to migrate them to Conan 2.x. Some of these packages have actions in their source method that require an external tool — for example:

  • Using CVS to check out source files from legacy source control.
  • Using CMake to run a time-consuming code generation step that's independent of the host context (and would therefore be redundant to repeat for each build configuration).
  • Using NuGet to fetch prebuilt binaries. (I think that this step does belong in the source method, because each NuGet package contains a collection of binaries for every supported build configurations: the desired NuGet package is therefore the same no matter the settings of the Conan package being built, and it's redundant to fetch it again for each configuration.)

I've packaged these external tools (CVS, CMake, Nuget) as tool_requires dependencies; this worked well under Conan 1.x.

However, it seems that the changes to how tool_requires dependencies work in Conan 2.x mean that this is no longer possible. Specifically, now that the tool_requires dependencies are made available via the VirtualBuildEnv generator (which can't be called from the source method), I can't see a supported way to use those tools in the source method.

The recipes do remain correct if I move these steps out of the source method and into build, but doing so means that a lot of redundant work is repeated for each build configuration. In some cases, this increases the time taken to build a set of packages from a recipe by several hours.

Is there any other way to use this tool_requires functionality from the source method in Conan 2.x?

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@memsharded
Copy link
Member

Hi @jdent-5c2ff9e

Thanks for your feedback.
This might not be specific to Conan 2.0, but to the way the new generate() and VirtualBuildEnv works:

  • The generate() works after source() method to generate information for the build
  • The information for the build is assumed to be configuration specific, not common to all configurations
  • The information provided by tool_requires in general cannot be assumed to be general for all configurations either
  • There are new checks in Conan 2.0 that restrict the usage of settings within source() scope, precisely to avoid the very common user errors of having sources depending on the configuration (which can not be done in the source() because it is general). This was a very frequent error in Conan 1.X and Conan 2.0 has strongly protected against it.
  • In Conan 1.X things might work because of a legacy injection of env-vars on memory. It might be disabled to emulate Conan 2.0 behavior with apply_env=False class attribute in recipes

So at this moment it is challenging to implement what you are trying.
But I understand the use case, and I think it is valid. I have started to have a look and I think it might be possible to provide this functionality without risking the protections of source() method against common errors. Let us have a look, I will take it to the team, think about it and I'll post here possible alternatives.

@memsharded memsharded added this to the 2.0.15 milestone Nov 21, 2023
@cirrus-haven-trimble
Copy link
Author

Thanks, @memsharded — that's much appreciated.

@memsharded
Copy link
Member

Hi @jdent-5c2ff9e

I have been working on #15153 to try to improve over this, but it is being more challenging than expected.

The case of creating packages in the cache with conan create is relatively easy. But the problem of the "local flow", in which developers run locally git clone + conan install + conan source + conan build is more complicated.

The strong assumption that source() should be constant and not depend on anything (not settings, not options), and it is even possible to execute the source() method of all dependencies and collect the sources with just a conan graph info ... -c tools.build:download_source, without even building the binaries, has been a big step forward in Conan 2.0.

But the same assumption that source() doesn't depend on anything make it challenging to use tool_requires, because tool_requires depend on settings, options, etc. Those tool_requires might not be even there at all as binaries in conan graph info.

For the local flow, we have proposed in the tests of the PR a possible alternative:

  • When there is no layout() defined, it works out of the box, because the first conan install will put the environment scripts just there and conan source will find them.
  • For the case where a layout() is defined, the following approach works:
def layout(self):
     self.folders.source = "mysrc"
     bt = self.settings.get_safe("build_type") or "Release"
     self.folders.generators = f"mybuild{bt}"

The trick is to define a "default" for the generators folder that will work even when conan source is executed, and no settings will be defined (build_type=None, so bt will default to "Release", it assumes that at least a conan install with Release has been executed earlier.

What do you think? Do you use the local flow a lot? Does this layout() sound reasonable?

@cirrus-haven-trimble
Copy link
Author

Hi, @memsharded. Thanks to you and your team for looking into this so quickly!

I see the logic of that technique in the layout method, and it does sound reasonable to me. I personally use the local flow when developing recipes that are slow to build (including the ones that motivated this question) — but I don't think it would otherwise be widely used within my team.

In cases where we'd otherwise implement layout using cmake.cmake_layout: do I understand correctly that I should avoid calling cmake_layout (because it requires settings.build_type to be defined), and instead replicate its action by hand (but with appropriate fallbacks when no settings are defined)?

I do appreciate Conan's efforts to ensure that the output of the source method remains constant -- and I agree that it's an acceptable trade-off to require a little extra effort in the layout method here in order to support the local development flow. 🙂

@memsharded
Copy link
Member

In cases where we'd otherwise implement layout using cmake.cmake_layout: do I understand correctly that I should avoid calling cmake_layout (because it requires settings.build_type to be defined), and instead replicate its action by hand (but with appropriate fallbacks when no settings are defined)?

Yes, exactly. If you don't need to cover all cases (build_folder_vars, multi-config vs single-config, subproject), then hopefully the layout is just a few relatively simple lines. But if that is not the case, let us know, and we can try to think something a bit more convenient.

I do appreciate Conan's efforts to ensure that the output of the source method remains constant -- and I agree that it's an acceptable trade-off to require a little extra effort in the layout method here in order to support the local development flow. 🙂

Thanks very much for your feedback. It is certainly sometimes extremely challenging to cover all use cases, that often even pull in completely opposite directions (in this case the multitude user errors because of conditioning source() to different inputs, versus the desire to use tool_requires in source()), so it is very appreciated when users understand these challenges and that sometimes their optimal preferred solution might not be possible and some compromises have to be made. Many thanks!

@memsharded
Copy link
Member

Finally implemented in #15153, will be in next 2.0.15, thanks very much for your feedback!

@memsharded
Copy link
Member

Hi @jdent-5c2ff9e

The solution in #15153 for 2.0.15 has introduced some regressions.

We are going to change it, and this opt-out is being changed in #15319 for 2.0.16, making it a opt-in. Youwill have to explicitly define source_buildenv = True in the recipe to activate the behavior. Sorry that we couldnt make it totally automatic, but the risks are high, and breaking existing users badly, so we need to be a bit more conservative here.

@cirrus-haven-trimble
Copy link
Author

Thanks for the update! The opt-in sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants