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

Fix/transitive build requires minimal changes #16849

Conversation

maximilianmuehlbauer
Copy link
Contributor

@maximilianmuehlbauer maximilianmuehlbauer commented Aug 20, 2024

Changelog: Feature: Propagate run trait requirement information in the "build" context downstream when visible trait is True.
Docs: conan-io/docs#3816

Fixes: #16333

This Pull Request is #16539 with more minimal changes. Requires in build context are now propagated depending on their visibility and run trait, but headers and libs are always set to False.

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.

Note that the expected value of the run trait had to change with some tests with packages in build context as this is now respecting what was initially set - for tool requirements, this is run=True.

  • 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: docs(requirements): propagate in build context docs#3816

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up on the review and contributing this PR.
I have been reviewing it, also added an extra test while running some checks.
I think this is much lower risk than the previous one, and I think it can be moved forward for next release. We will probably need to document this a bit more and mark it as experimental just in case, but seems this is a more correct behavior.

@maximilianmuehlbauer
Copy link
Contributor Author

Thanks for following up on the review and contributing this PR. I have been reviewing it, also added an extra test while running some checks. I think this is much lower risk than the previous one, and I think it can be moved forward for next release.

Thanks, that sounds good!

We will probably need to document this a bit more and mark it as experimental just in case, but seems this is a more correct behavior.

I've started documenting it here: conan-io/docs#3816

One more thing might be to disallow or print warnings when headers=True or libs=True is set for build=True in the first place, what do you think?

@memsharded
Copy link
Member

One more thing might be to disallow or print warnings when headers=True or libs=True is set for build=True in the first place, what do you think?

I think at the moment I am fine with not propagating them. An error/hard disallow will probably be too much, one of the problems of having such a large userbase is that it is likely that there is someone out there relying on being able to do that, and we will break them. And warnings and other information have relatively low value, practically no ones reads them...

So probably it is not worth the effort, until some users (repeatedly) report issues or confusions being hit by this.

@maximilianmuehlbauer
Copy link
Contributor Author

One more thing might be to disallow or print warnings when headers=True or libs=True is set for build=True in the first place, what do you think?

I think at the moment I am fine with not propagating them. An error/hard disallow will probably be too much, one of the problems of having such a large userbase is that it is likely that there is someone out there relying on being able to do that, and we will break them. And warnings and other information have relatively low value, practically no ones reads them...

So probably it is not worth the effort, until some users (repeatedly) report issues or confusions being hit by this.

Okay, I've added that to the documentation pull request. Please let me know if you need more input / changes either here or in the docs pull request.

@czoido czoido merged commit b1e70ba into conan-io:develop2 Aug 27, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] requires of a dependency of a dependency in build context not visible
3 participants