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

• 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.

[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

conan/conans/model/requires.py

Lines 271 to 275 in cb106f0

	if require.build: # public!
	# TODO: To discuss if this way of conflicting build_requires is actually useful or not
	downstream_require = Requirement(require.ref, headers=False, libs=False, build=True,
	run=False, visible=self.visible, direct=False)
	return downstream_require

changes the traits when propagating a requirement in build context which looks a bit odd to me. The second if

conan/conans/model/requires.py

Lines 277 to 283 in cb106f0

	if self.build: # Build-requires
	# If the above is shared or the requirement is explicit run=True
	if dep_pkg_type is PackageType.SHARED or require.run:
	downstream_require = Requirement(require.ref, headers=False, libs=False, build=True,
	run=True, visible=False, direct=False)
	return downstream_require
	return

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 f734566 (from #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:

conan/conans/model/requires.py

Lines 267 to 269 in cb106f0

	if require.visible is False:
	# TODO: We could implement checks in case private is violated (e.g shared libs)
	return

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.

[memsharded]

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.

[memsharded]

Propagation of visible=True is not only about traits, packages do not only conflict because having headers, even if they don't 2 different packages (for the same package name) with visible=True will conflict.

If they don't conflict, there could be 2 different version of the same myscripts/version package being propagated down the graph as visible, but not conflicting would be a problem.

[maximilianmuehlbauer]

Hmm okay but shouldn't then requirements always be propagated when they are visible? This is definitely not the case in the current implementation.

[memsharded]

This change sounds problematic. It means it is propagating headers and libs from the build context to the host context, which will be broken in majority of cases. I don't think that headers and libs that might be in a package containing an executable that we are using via tool-requires and some user decides to make it visible=True to propagate such executable should start receiving headers+libs of a potentially different architecture, as they will result in compilation and/or link errors. Note this trait means" the libb is contributing headers and libs to app, which wouldn't be correct accross contexts.

[maximilianmuehlbauer]

Yes that definitely makes sense. But shouldn't in that case headers, libs, transitive_headers, transitive_libs always be False in the build context?

[maximilianmuehlbauer]

Should that maybe be enforced somewhere?

[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 #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 #16849 has been merged instead