-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[docs] Add a policy about patching #3903
Comments
I welcome this. #3834 is not the first time that we had discussions if it is okay to apply certain patches or not. Having a policy will certainly help. I mostly agree with the points above, but I would also like to keep the door open for back porting of reasonable large patches (which I guess is hard to define) which fix bugs in the libraries/tools, when those are already accepted upstream and will be part of a next release. Sometimes it can take a year or longer for new releases to come and if there is already an accepted solution which might be easily portable (e.g. only very few line changes) then I don't see a reason why we should deny this in principal. I would then leave it to the educated guess / feeling of the reviewers if something looks reasonable or not. If the project is getting maintained but a patch is not accepted (yet), then there is probably a good reason and we should not apply the patch.
I agree with this first part
But not with this one. I think we have to differ
|
Croydon, would if be fair to summarize your position as "follow as upstream does"? If the patch was accepted and merged into the default branch, it could just be a virtual version as well? I think this would prevent consumer from having unwanted surprises. |
you do not seriously want starting to discuss if it is OK to add a patch that fixes a clear bug in a package that prevent the package to work properly? because if you think bugfixes are no reason for a patch than I will stop immediately use cci and work on the packages I am interested in my own fork. therefore, patching of builds, without providing the changes upstream to the original source, this is really bad practice! Unbelievable what this ridiculous reaction of a library author who felt piss off because someone touched his 'private recipe' caused for a waste of energy. The whole discussion was clearly not for technical reasons, because technically there is zero value in the complains. and btw, #3834 has still no review and is hanging. a good way of demotivating people that share work. |
Hi! From the POV of a package manager we cannot modify the behavior of a library when the reference matches a released version. We shouldn't add patches that fix bugs or add/modify features, otherwise the packaged binary will no longer match the expectations of the released library, it will no longer match the changelog and, it can be very confusing to consumers and to library authors if issues start to arrive to the upstream repository. If we add patches, we will add one today and one tomorrow, and different recipe revisions will behave differently. Vulnerabilities are a different kind of patches, we can talk about them in a different way. When the upstream library is not maintained or there is strong evidence that it will take too long to release a new version with some pending patches, my suggestion would be to create a new version based on an existing one and apply patches to it. That way, the canonical reference will match the released library (with its bugs, that some project might be using like actual features) and this new version can contain patches that have been already submitted/merged for the next release in the upstream repository. We already have |
sorry to bother you @a4z but I opened this issue to collect some feedback about the points above and try to reach some consensus around patching in recipes. First of all, I would like to clearly state that we consider all the views whenever we discuss a topic, no matter if they are coming from advanced Conan users, beginners, library authors, recipe contributors... Everyone will always be welcomed with constructive feedback because is the only way to make a project like Conan and ConanCenter work. Secondly, here I was talking about general policy and I am not talking about the specific case in #3834. In fact, I strongly believe that the discussion happened because we did not have this kind of policy and as @Croydon mentioned, it is something needed at this point. Specifically talking about patches for bugfixes, we all know that there are always bugs in software and it is something we have to live with. However, I can also see the pain in changing the behavior (even if it is a bugfix) for someone that is relying somehow on it. I am not saying that we should not patch things, I am saying that we should set a common ground in the conan-center-index so everyone knows what to expect then they consume a package from ConanCenter. This means that there are patches that everyone could benefit from based on these rules and probably other ones that certainly will have to be maintained in forks. So finally, to answer the last point, I don't think #3834 is a waste of time. In fact, it helped to start this discussion and it will eventually help to review the PR with common criteria from the reviewers. |
if things like in #3834 will not be applied, cci becomes worthless for production, and if you think this is cool, this means the few people that actually do notifying you about such problems will stopp bother. and then you do not need to wonder when you not get any fee back from user that are normally in a less open space (company) than the opens source space, because for getting this you need to be able to distribute working software form that blog
Insist on shipping of non working software via the cci and create more other reasons In none of the changelog or howto or what ever of paho-mqtt-c was mentioned that the current behaviour is like the bug does, quite the opposite is the case, it should work, and with the patch it does as announced. So sorry Javier, what you write makes no sense. at all. And you do not bother me Daniel, add this policy, make it clear that you reject to deliver working software and prefer to ship software with known bugs via cci , and you will spare me quite some time in future, and that is good to know. |
something to consider:
|
Good data @SSE4
Discussing "categories of patches" only matters if we decide "some patching" will occur. Quick points on theory:
Quick points on practice:
User Expectations:
It's reasonable to say that most users today have the most of the following context:
I think our top priority right now should be to ensure that when users do research, they can easily find clear explanations on what the policies are (whatever we decide them to be). Then, whether we patch or not, users can make informed decisions. Clarifying policy: There's obvious opportunity to add statements to CCI readme, conan center index web page, search results, etc. Additionally, I would suggest starting a discussion somewhere around adding code (potentially hooks), which outputs policy and disclaimers in the CLI output whenever packages are installed from Conan Center Index. Maybe even when they're consumed. |
good you mention the theory of "Only official sources ever" , @solvingj because #3834 fulfils that, for the patch I added, not the patches of the cmake things. since I created the patch for official sources. still, my change is under discussion and now frozen and topic of bureaucracy and has after days no review because of a special lib author whose comment should be deleted, and I regret heavily pinging this individual for a review, since he clearly misses competence on the topic software delivery, because if he would have some, he either accepted the patch on the package happily, or release an new minor version of his buggy software quickly containing the patch. so, anyone here involved once to give #3834 a positive review, after seeing that this would of course also work for ubunut, yocto and ot her things that have some idea about packaging and delivering software, in opposite to the author of the effected library, and after seeing what a internal small thing that patch is, and that this is even accepted for the next version without discussion..... |
Firstly I apologize that you have recently had a rough time with your PRs. I absolutely understand it can be frustrating. Working with the community for a few months now and I want to assure we take your contributions unequivocally as all others. Now to address your comments
This is very common. Right now there is an above average number of pull requests open. I personally have some that have not been reviewed and other getting stale like #3711. Absolutely every contribution is being affected by a slow down.
On my behalf, I am not with holding my review because of the comments by any individual. I need the topic at large needs to have a consensus before being able to move forward. I do not believe my opinion should be the basis for a review but rather an objective qualification of the work presented. At this time I do not feel I can meet that standard.
I love human perspective. I read those links shared and a few others from searching quickly, and I am under the perception that your changes are not accepted by those package managers. |
From Ubuntu: what is exactly the use case! And I know about patches that made in into corporate, kind of proprietary, yocto Linux dists that run on system that need certifications, to give you your company background also, but you will have of course your own human perspective ;-) so feel free to create your interpretations as you wish. |
meanwhile in the cci , 9 days of shipping software with known bug instead of applying an easy fix. |
good luck reviewing all the, and throw out those packages that patch source code, or remove the patches that modify the code :-) an other prove that you are just confused by the bite reaction of a person who took it personal that someone worked on his recipe, and tried to confuse you with world most stupid arguments on that topic @jgsogo , I think it is time you ack my PR. Or, remove all recipes with patches that modify code. I found some where the changes are more massive and complex than the very clear and simple patch I added #3834 . Or shall I think you apply double standards? Here a list with all patches, enjoy the review :)
|
Of course, @a4z , if we agree on a policy and it doesn't match existing recipes we will work on them and fix them. It is exactly what we are already doing when we introduce a new hook or any other policy. This is a collaborative effort, no one of us can take it alone. I think there are very good reasons and opinions here and in Slack, we try to gather all the feedback and propose something coherent and useful for all, now and in the future. |
Hi @jgsogo ,
don't forget to review all 433 where people use tools.replace, and remove those that modify sources, because people do that since they need working software it turns more and more out how in what a ridiculous corner you put yourself by letting you confusing of a total incompetent person just because that person is maintainer of a project nobody else wants to maintain, who is total incompetent when it comes to the topic of software delivery , and who had not maged to update and release a new version of his known to be buggy software for very long time now, more time than industry can accept. |
@a4z I'm not going to go personal on this. If you want to, you know you can find me on Slack and sometimes we've been live in the same conference and/or space. Feel free to approach me and we can talk. Same about everyone else, all people here are quite approachable and willing to help. After this COVID-19 that is taking its toll on all of us, I am confident that we will celebrate a ConanDays together, toast and congratulate ourselves for an effort that is bearing fruit. Please, do not label someone as incompetent, there are many personal circumstances and I'm sure everyone is doing their best when working on OSS, especially if they are working for free on it and/or in their free time. This is also what I think about you and what you do, you are doing your best and you try to help other people here, and I'm really grateful for your comments, your contributions and your help to many users here. I'm being paid, Conan is my job and ConanCenter is something I do because I love it, but also because I'm paid for it. But I know that this repository wouldn't be possible without all the help of so many people who are working here for free (you included) and I smile and get excited each time I think we are changing the C++ ecosystem, I really think we are changing the future for C++ developers. I would love to write more C++ and I'm sure I will come back to it sooner or later... but at this moment I'm living a dream trying to build the tool I needed two years ago and the one I will use in the future together with an amazing set of libraries available and ready to use. We need a policy about patching, it can be more or less aligned with your thoughts or mine, with what you need today or tomorrow, with my needs or yours... but I'm sure it needs to be aligned with what ALL OF US (maintainers, reviewers, contributors, authors and consumers) need and what we can afford maintaining in the long term. Tomorrow you and I will be at the other side, only consuming the recipes other people are submitting to |
Which ones? |
take a look yourself SpaceLm, I posted the list of current patches. There are so many patches with no explanation at all, but by my one people freaked out because they listened to a hostile and confused library author. and sorry Javier, I just wanted to be helpful and point out that you have not thought on all the side effects you cause, and how discriminating you behaviour is (not to mentions Ians behaviour) against those users are who are effected for the bug in paho-mqtt-c , since there is currently no version available on the cci that is usable if you have certain workflows. |
That example is a build tool. The explanation is in the patch...
And it was specifically added to enable it to work with conan's plentiful compiler options and default paths. Which makes perfect sense. There's a 2.6.1 that was released last year 🤔 but the downloads are no longer available on sourceforge! Where did they go?! There has been several issues unaddressed or PRs close/altered simply because they were trying to apply patches that seemed unorthodox. Recent example #3918. There's too many closed issues for me to find them now but feel free to some research. You are not the first to have a PR stalled due to a patch. Please have some patients we are trying to resolve the issue. Congratulations for raising this issue and bringing lots of attention t o it, we now have quorum to form a consensus. |
there is always some explanation behind double standards, so lets do not go into this, it is also too boring to go through all existing code patches, do this on your own.
Call me crazy, but if you seriously dam me because I am passionate that cci stays to be, and becomes even more, a place where high quality libraries are released, I do not care. anyhow, I have repeated my self now often enough, I try to get this other eclipse mqtt library to work , and will not invest any more time on this 'issue' or the paho-mqtt-c pull request. |
Why do you think things like BoringSSL exist? |
Related to the discussion at #3834
We should all agree on some general policy around source code patches in libraries.
To start the conversation I would add the following points:
Those are just first thoughts, so feedback is welcomed.
The text was updated successfully, but these errors were encountered: