fix(requires): don't stop propagating requires in build context #16333

[maximilianmuehlbauer]

Changelog: Fix #16333 This Pull Request treats requires in build context just like normal requires, i.e. their propagation isn't stopped arbitrarily if visible=True is set. Note that build_, tool_ and test_ requires default to visible=False so this should only change things if users explicitly put a requirement in build context and set visible=True.

The proposed changes fix the example in the linked issue and also our local Simulink workflow. Please let me know about additional checks / use cases to be performed or where unit tests could be added.

Docs: https://github.com/conan-io/docs/pull/XXXX

• [x] Refer to the issue that supports this Pull Request.
• [x] If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
• [x] I've read the Contributing guide.
• [x] 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.

[CLAassistant]

All committers have signed the CLA.

[maximilianmuehlbauer]

2519f44 summarizes the changes - requires in build context don't change having headers, libs, run etc. set when being passed downstream. Please let me know if that is okay - at least libs and run would be needed for my use case, I think.

[memsharded]

Hi @maximilianmuehlbauer

Thanks for contributing this. I have been wanting to move this forward a bit, but difficult to prioritize it high enough.

The fact that are tests changing behavior is something that we need to carefully investigate, to see if something could be breaking. Specially how it affect other traits. I'll try to have a look at this in the following weeks, thanks!

[maximilianmuehlbauer]

The fact that are tests changing behavior is something that we need to carefully investigate, to see if something could be breaking. Specially how it affect other traits. I'll try to have a look at this in the following weeks, thanks!

To my understanding, this is very much to be expected. This first if https://github.com/conan-io/conan/blob/cb106f045c751d7bfd2ce8f80b4056189073d809/conans/model/requires.py#L271-L275 changes the traits when propagating a requirement in build context which looks a bit odd to me. The second if https://github.com/conan-io/conan/blob/cb106f045c751d7bfd2ce8f80b4056189073d809/conans/model/requires.py#L277-L283 changes traits from a requirement of such build requirement and breaks the dependency chain by setting visible=False.

I do not fully understand why one would want to change traits when transforming a requirement downstream, maybe you can point me to some docs / other code? Using git blame, I've also stumbled over https://github.com/conan-io/conan/commit/f73456601e9eca8e2a7c12e7d7cebbe1ca98bb6b (from https://github.com/conan-io/conan/pull/15357) so probably this would be a continuation of #15357 in the case of longer dependency chains.

Note that stopping propagation when visible=False is done right at the beginning of transform_downstream: https://github.com/conan-io/conan/blob/cb106f045c751d7bfd2ce8f80b4056189073d809/conans/model/requires.py#L267-L269

As tool_requires and build_requires both specify visible=False according to the docs, they won't be propagated downstream. The only case where this Pull Request really changes things is when you explicitly specify build=True and run=True.

[memsharded]

To my understanding, this is very much to be expected.

Sure, I am not saying that the changes look incorrect. Still for any changes like this we need to carefully analyze possible impact on users relying on that "incorrect" behavior and the ways they could be broken. Then, if it is what makes sense we can declare it a bug fix and move forward with it, it is just that we need a little time to review it, so it doesn't look possible to squeeze it in this release due in a couple of days, and it is planned for the next one.

[maximilianmuehlbauer]

Dear @memsharded, is there anything more we could test? In particular, is this the correct target branch and will those changes land in 2.6.0? In that case, I'd update the branch and we might consider already rolling it out locally to check for any potential flaws - this should be the last showstopper before switching to conan 2 for us.

[memsharded]

Dear @memsharded, is there anything more we could test?

Thanks @maximilianmuehlbauer for the ping.

Everything is good, the branch is look, I am just trying to allocate some time for this, but it is being challenging, too many things on our plate, and this requires some quite deep consideration, cannot be done lightly. It is scheduled for next 2.6, hopefully we can have a look in the following weeks.

[maximilianmuehlbauer]

We have been considering this, and we think this is a bit too risky, and the cases for making visible "build" requires and propagating them downstream need a different consideration than regular requires, then don't follow the same rules.

We still understand that there might be some cases that could benefit from having propagating build requires, but we think it is necessary to build from them, from the real use case up, and not by changing the current logic and changing the affected tests.

The recommendation would be to start with some "real" use case in a form of a new test, that requires one or several tool-requires to be visible=True. It is fine if the test is initially broken, we don't need the solution yet. From there we could work and define the right propagation logic for build requires, satisfying the real use case, and take an incremental approach from there, satisfying the use cases with the minimal "safest" changes.

I have now opened https://github.com/conan-io/conan/pull/16849 with a more minimal set of changes based on a new test with complex requirement tree; specifically headers and libs are now always False. I have only touched the propagated run and visible traits - the run trait specifically because I think that those should respect what was set with the initial requirement. There might be workarounds to that though, if event that is considered too much of a change - but in that case, I think it gets unclean. I have verified this new Pull Request with some of our Simulink models and it works as expected. Please let me know what you think.

[maximilianmuehlbauer]

Closing as https://github.com/conan-io/conan/pull/16849 has been merged instead