[docs] Add policy about patching #3951
Conversation
This comment has been minimized.
This comment has been minimized.
| should raise a `ConanInvalidConfiguration` from the `validate()` method. | ||
|
|
||
| **Vulnerability patches.-** Patches published to CVE databases or declared as | ||
| vulnerabilities by the authors in non-mainstream libraries will be applied |
There was a problem hiding this comment.
| vulnerabilities by the authors in non-mainstream libraries will be applied | |
| vulnerabilities by the authors will be applied |
These seems like an unnecessary addition
There was a problem hiding this comment.
Active projects will apply them (in a timely manner 🤞 ) so we should not need to handle that case. I am okay with the original text
|
This policy would probably be stricter in some cases as I would have personally suggested, but I understand the arguments and I'm okay with it, as long as we are less strict about these things when it comes to Another exemplary edge case to consider: Assuming an application does depend on OpenSSL. However, the package does only support an OpenSSL version which is end-of-life and has known vulnerabilities/CVEs. Someone proposes a patch that makes the application compile and work with still supported OpenSSL versions. The application itself has no newer releases which could be added and used instead. Would this patch count as a vulnerability fixing patch and be therefore accepted or would it count as a supporting-newer-dependencies patch and would therefore be denied? Personally, I would count this as a vulnerability fixing patch which is important and shouldn't be generally denied. However, as with all patches it should proportional. Meaning if the patch would have a few thousand lines of code or something like that then we can't reasonable maintain this. |
| considered feature patches and they are not allowed either. They can | ||
| introduce new behaviors or bugs not considered when delivering the | ||
| library by maintainers. If a requirement is known not to work, the recipe | ||
| should raise a `ConanInvalidConfiguration` from the `validate()` method. |
There was a problem hiding this comment.
Maybe wait for conan 1.32.0 in CI?
Also, why raise only in validate(), when we can raise before dependency graph resolution (saves from potential download of useless recipes)?
For me, the new method validate() is only for verifications based on informations unknown before dependency graph resolution (options values and versions of dependencies).
There was a problem hiding this comment.
Hi! The final version of the requirements is only known after the graph is fully resolved. There can be overrides and diamonds, the final version is not known in the configure() method while Conan is building the graph. I agree this would only happen with version ranges and with a few modifications in the graph resolution algorithm (a challenge for Conan 2). Now it will raise a conflict or not depending on the order of the requirements listed in app:
I agree this is not a scenario in conan-center-index where the version of the requirements is hardcoded.
| Patches to sources to add support to newer versions of dependencies are | ||
| considered feature patches and they are not allowed either. They can |
There was a problem hiding this comment.
🙊 broken by websocketpp to allow recent version of boost while maintainer was MIA
There was a problem hiding this comment.
Replies to Croydon #3951 (comment)
|
I'm converting the PR to a draft so it doesn't get merged accidentally. |
|
It is hard to make a clear distinction between security patches, bug/feature patches and even build patches. Packaging and patching OSS in well studied and practised in Debian, for example. Description and origin of a Debian patch are tracked by DEP-3 format metadata. Upstream and forward status (whether the patch was applied or sent in upstream) allows maintainers to track PRs. There is even lint tooling around DEP-3. Forcing this complex format for recipes can be overkill. Probably we should use a similar description (structured or free-formed) for our patches to understand why a given patch is here and when it can be removed with a new release without searching in CCI or upstream issues. By introducing quite large fixes for newer C++ standards and rare compilers we already change the code and behaviour. It can be changed explicitly by adding backports or security fixes. Since a package version 1.2.3 does not change with patching, it is already hard to tell if a package can still be called of version 1.2.3. If patches are properly described and categorized, I see no problem with security fixes, bug fixes and backports too. As we can see, a new release can be awaited for a long time or upstream may be dead. Of course, a delta with upstream should be kept as minimal as we can for all kind of patches. Proposed changes to the policy:
|
It's intended to fix a CVE? Certainly in favor for leaving a trail in the code 👍 |
|
Quick suggestion: if a patch makes the binary deviate from upstream's version, Could it be ok to make it opt-in by using an option (with_patches or something else) which is False by default ? |
|
would be interesting to see how npm and cargo handle this... |
To my knowledge, NPM just tracks the fix versions based on the CVE database when a fixed version gets published. example I really like your idea Eric!
Perhaps a virtual release? piggy back of the existing system? This way we can maintain the consumer expectation of |
Originally posted by @ericriff in #3918 (comment) |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This PR is not to close, but we are waiting for some _technical details (conan-io/hooks#268) before merging it. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions. |
|
This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions. |
|
I think we should have the policy in place to handle issues that may arise in patches contributed via PRs. So I am reopening this PR to continue its approval process |
|
needs a rebase |
|
Updating docs! |
|
let's move this forward and polish the technical details at conan-io/hooks#268 in a a new PR |
Co-authored-by: SSE4 <tomskside@gmail.com>
We need a policy about patching that is sustainable in time regarding libraries and reviewers (thanks again). We read feedback from issues here and also from comments in Slack and this is the proposal we think that can be most adecuate for regular consumers of Conan packages from ConanCenter.
close #3903
This is a live and thriving community full of passionate people. I'm personally very thankful to have you all around. Thanks! 😊