search

conan-io / conan

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

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

Open maximilianmuehlbauer opened 5 days ago

maximilianmuehlbauer commented 5 days ago

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

CLAassistant commented 5 days ago

CLA assistant check
All committers have signed the CLA.

maximilianmuehlbauer commented 5 days ago

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 commented 4 days ago

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 commented 2 days ago

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 commented 2 days ago

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.