[service] Why PRs review is so slow?

[ilya-lavrenov]

What is your problem/feature request?

Hi,

Who are CCI maintainers so people can ask them for review? All my PRs are reviewed with very slow speed and I don't know what to do with it.
Can you create a group of maintainers which is auto assigned to review? There are several strategies on GitHub and people from maintainers group can be assigned one by one to have a balance.

PR #17786 is created more than two weeks ago and it's green since the beginning, but nobody performs review and I have to ask people (I believe in a good community I don't have to do it). But nobody cares, nobody review the PR. Why? Do you think people create PRs just to have a fun? No, they create PRs, because they want to use merged changes in their products. But if I blocked with my changes for weeks, I don't want to use Conan and prefer to use vcpkg, where review is performed faster.

From Conan contribution guide:

The Conan Community works hard to review all the pull requests and provided assistance where need

It's definitely not a truth.
I understand that everyone is busy, etc, but it's not acceptable when PRs with version bump take weeks to be merged. Can you create automatic at least for such cases?

[jcar87]

Hi @ilya-lavrenov - thank you for opening this issue.

Please accept our apologies in the delay. We have a high volume of pull requests in Conan Center, and indeed the Conan 2.0 release and the migration efforts have added an additional burden in terms of resources (both infrastructure resources and review efforts). The team has a number of responsibilities and we are doing the best we can to ensure we can provide this service.

On the subject of adding new versions, these are fully automated as described here: https://github.com/conan-io/conan-center-index/blob/master/docs/bump_version.md - however if the recipe contains changes other than adding a new version, it will not be automated and will require a review from the Conan Center maintainers. - I believe that is the case with #17786

We are continuously working to improve the service and we are evaluating how to best prioritize pull request reviews - we thank you for your feedback.

[ilya-lavrenov]

Hi @jcar87

Thanks for your answer. Magically, PR is merged now 😺

But it's not a single PR, I have another and plan to do more.
What is recommendations here? What as a contributor should I do to ensure PRs are reviewed and merged faster?
Is there a list of maintainers I can ping to help with review?

We have a high volume of pull requests in Conan Center

I understand that we are all busy, don't get me wrong. But in order to grow community, maintainers should be responsive to external contributors. If I have reaction on my PRs - I want to contribute more, it means less work for you as a recipe writer and more as a PR reviewer. If you still write a lot of code by yourself and don't react on contributions, you will write a lot of code in future as well IMO.

Good example is brew package manager - they react to changes within 1 day.

[jiaoew1991]

I have a similar problem. The PRs for several recipes that I've been following have been ready for almost a month, but they haven't been merged yet.

We don't want to use our own JFrog to manage packages and would prefer to use the official Conan Center instead, but the merging process is just too slow 😅.

[ilya-lavrenov]

Hi @RubenRBS @jcar87 @memsharded

I have another PR where review takes a lot of time. Every two days new reviewer comes to PR and write really small comments and ask questions:

• Why does this PR have even 5 reviewers? How many people should open PR and write comments to finally merge the PR? Don't you really trust to each other, so every new reviewer re-checks after pervious ones? Why 1-2 reviewers are not enough? Why people who left review never open PR again and approve it?
• Please, separate comments into two groups: must-have and nice-to-have. I see a lot of comments from the second groups, please, try to minimize them. We should not spend months with idealizing every recipe, because it's an open source and new recipe users will submit improvements if they are required. If someone needs new capability, she / he can add it via submitting PR. I suppose it's better to have recipe which is 90% ideal than not to have it at all, is not it? I'm actually ready to make my recipes 100% ideal if the review process will not take weeks or months..

Again, if you don't react on contributions or complicate life of contributors => don't expect community grow around your solution.

[SSE4]

my 2 cents, just imho.
I suppose minor issues like code styling shouldn't block reviews and shouldn't be really mandatory to apply. most of these issues are caught by hooks and linters already, but if they aren't, then linter should be improved.
it's okay to apply these side-changes if you're doing other logic/functional changes anyway, but otherwise, they are not strictly needed, in general.
in general, PR should be blocked only for reason it's doing something wrong from the logic point of view (in other words, consumer's viewpoint) - e.g. breaks existing configuration, or otherwise making destructive changes.
minor things like order of methods, topics, style preferences, etc. could be fixed in separated follow-up PRs, it shouldn't be mandatory to make 100% perfect polishing - better to have some recipe with incorrect code style rather no recipe at all.
I usually follow a simple concept that PR should be atomic - doing only one thing at time, and doing it well. if it adds new version or new configuration or new library - it's just fine if PR does only that, not including code style or otherwise any other minor nice to have fixes.
if PR passes CI and doesn't do a completely wrong thing from functional perspective - it should be accepted.

[ilya-lavrenov]

@SSE4 fully agree with you. I'm also a maintainer of another open source project and know that each project has its own specifics. And it's impossible to get from external contributors the code with the same quality like experienced maintainers wrote it. Or it will take 100 iterations with comments to every 10 lines of code, which will likely result in abandoned PRs. So, maintainers should make a choice: not 100% ideal external contributions or no contribution, but all the code is written only by core team.

Regarding minor comments I have received in my PRs:

• topics duplicates recipe name, but if you grep over Conan recipes - you will find a lot of examples where the same is done. Do you really believe this is useful attribute?
• questions like "why this capability in the library is turned off"? (note, it's happening in a new recipe). The simple answer - because I don't need this capability for my purposes. I suppose it's normal that if library has optional capabilities and I need only several of them, then I will add options only for capabilities (read dependencies) I need. If someone needs extra capability (read dependencies) in future - he / she can contribute and add explicit recipe key to enable specific library feature. Reviewer should understand what recipe writer in most of cases is not the library developer, it's a different person who just need recipe in Conan, so he / she cannot answers all the questions why this or that dependency is used.
• It's clear that in 99% share folder contains some auxiliary files (docs, examples code, auto-completions files, config for some tools valgrind, etc, cmake scripts - removed in Conan), which are not needed in runtime (and a lot of recipes remove this folder), but for some reason reviewers still ask "why share folder is removed"? If reviewer is really interested in content of share folder, he / she can build the library locally and observe everything by himself / herself. I believe recipe author does not have to provide review about library content, how it works or what files are required. Library public documentation can serve for these needs.
• "Test package should be simplified". Really? The test consists of 4 function calls from the library (I took a very first sample with minimal number of code). I believe the goal of the test is not only ensure that include headers can be found and symbols are resolved by linker, but also run minimal amount of code to guarantee that library in bound of Conan distribution at least not crashes (read well-configured)

[SSE4]

@ilya-lavrenov yes, for point 2, specifically - new recipe doesn't need to be 100% complete. if it builds only for Windows in its first iteration - it's completely fine. if it doesn't enable/handle all the options upstream has - it's perfectly fine. if not all targets or components are modeled in package_info - it's also fine. if not all possible dependencies are added - still fine. having some recipe is still more preferred than having no recipe at all. additional configurations, options, features, etc. could be added on demand in a follow up PRs.
following that strategy could accelerate reviews significantly, and keep people (both reviewers and contributors) focused only on the relevant details.

for the questions asked in review - they might require quite a significant research. especially, for newcomers, who might not fully understand what is share directory at all. in this case, all the possible answers to this questions shouldn't have made PR blocked, so raising the question in the first place is not so vital for a review.

[AbrilRBS]

Hi all, let's try to break this down little by little to try to address most of your points:

First of all, thanks for raising the issue about slow review times, we appreciate feedback on how contributors experience their CCI flow

@jiaoew1991

We don't want to use our own JFrog to manage packages and would prefer to use the official Conan Center instead, but the merging process is just too slow 😅.

Note that it's pretty encouraged not to depend on ConanCenter for your production code and recipes. The recommended approach is to have your own fork of ConanCenter repository where you build and store the necessary packages for your organization, ensuring you can create edits to them in a much faster way, and having better control over which packages are available at any point (You might have configuration requirements CCI does not offer, for example) and then upstreaming the changes as a separate process

@SSE4

I usually follow concept that PR should be atomic and doing one thing at time. if it adds new version or new configuration or new library - it's just fine if PR does only that, not including code style or otherwise any other minor nice to have fixes.

new recipe doesn't need to be 100% complete. if it builds only for Windows in its first iteration - it's completely fine

It's fine for PRs to only do that if thats their scope, absolutely! We actually encourage contributors to make it clear when they have not been able to test a specific configuration, and to raise ConanInvalidConfiguration for those cases asking for PRs to add support, because as you said, we understand that most users don't have access to all the configurations CCI ends up building with

if PR passes CI and doesn't do a completely wrong thing from functional perspective - it should be accepted.

I have to disagree with you here. You most of any other contributor is aware of how much work goes behind every review, and that ensuring that changes are good is not a trivial task most of the times, we're not here just to press the merge button based on good faith on the contributor changes

@ilya-lavrenov

"Test package should be simplified". Really?

Yes! Don't only look into this as trying to simplify for simplicity shake's, this is also a question of reducing CI workload. Compilations absolutely dominate our work time for the build agents, and that we can do little about, it's the whole point of the system in the end, but what we can do is reduce tests runtimes which also get run quite a few times, so every simplification we can get there is worthwhile.

Why does this PR have even 5 reviewers?

Because reviews are dismissed after every new change (after all, you don't want to approve a recipe only for it to get broken/a bad faith commit later on and for it to still get merged), and the amount of PRs that we receive means that unless we get a direct notification once the PR is ready again after a positive review, we might miss it and not re-review it until we notice it again, or someone else from the team does. Also note that there are actually two kind of reviewers in CCI. Community reviewers (If any of you are reading this, thank you for your contributions), which help us a lot by also double checking PRs and finding issues (And others contribute actual recipes quite a lot!), but whose reviews are not actually taken into account by the Automatic merge bot to understand when a PR is ready for merge, and the core team members, for which you need at least 2 of for a PR to be mergeable. You can find the list of users that are at any moment part of any of those groups here. It's true that this difference is not well communicated to the contributors and we'll look into ways to have it more clearly explained to the users so there's less future confusion about it :)

Don't you really trust to each other, so every new reviewer re-checks after pervious ones?

We do! The team has great insight into lots of things, but the value of independent reviews is, as I mentioned, to be able to catch things that someone else might have not, so trusting that the last person did their job is in my opinion dangerous! (Even if I trust them a lot, which we
do!)

---

(The next responses except the ending paragraph are written alongside @jcar87)

@SSE4 fully agree with you.

But I have received a lot of comments like:

I would clarify that you have received review comments, which is our job as reviewers. From our maintainers point of view, it is our job to ensure that recipes and any contribution in the repository is vetted before being merged. This is, to my knowledge, the case with similar C++ packaging repositories where the recipes are open source. Please understand that most of those comments are coming from a place of actually wanting to understand what's going on with the specific recipe, not out of spite to nitpick in small things!

I'm actually ready to make my recipes 100% ideal if the review process will not take weeks or months..

We appreciate this a lot, and we are actively working on ways and processes to be able to reduce the delay time for reviews (But again, never at the cost of actual recipe quality!). I can't speak for everyone else, but for me personally, when I request changes for a recipe that seem minor is not because those changes need to be there, but because my review was not a complete one and I don't want it to get merged without me having a chance to go back and finish that, but I don't want to also leave those messages in limbo. It's true that we can communicate our intentions better, and it's a point we're trying to improve upon :)

• topics duplicates recipe name, but if you grep over Conan recipes - you will find a lot of examples where the same is done. Do you really believe this is useful attribute?

Conan Center has been evolving since the first day, and so do the conventions used in the recipes. We are aiming towards simplifying the logic of the recipes in the long term - without burdening our contributors with ensuring all recipes are up-to-date with the most recent conventions. The pull request you are alluding to is a new recipe - what we expect for those, at a minimum, is that it follows the current template in the documentation. We typically won't ask - and certainly will not block - for these sort of changes in PRs against existing recipes. Having said that, as I explained in the PR, we're actively investigating ways for contributors to have less friction with issues like this in the future, and we appreciate your input in the matter

• questions like "why this capability in the library is turned off"? (note, it's happening in a new recipe). The simple answer - because I don't need this capability for my purposes. I suppose it's normal that if library has optional capabilities and I need only several of them, then I will add options only for capabilities (read dependencies) I need. If someone needs extra capability (read dependencies) in future - he / she can contribute and add explicit recipe key to enable specific library feature.

This is an interesting take - you are saying the first person "I" several times. Conan Center is a service that we provide to the C++ community at large - not to any particular individual. And as reviewers, it helps not only ourselves but future contributors to have minimal traceability and reasoning behind some decisions that deviate from the defaults. You are asking that reviewers accept your changes according to your needs, what we ask as reviewers is to assume that this will be used by more users, and to try and provide value not just to you, but to everyone that may use this recipe in the future. Note that we're not asking to implement those features, as you say future contributors can do this.

Reviewer should understand what recipe writer in most of cases is not the library developer, it's a different person who just need recipe in Conan, so he / she cannot answers all the questions why this or that dependency is used.

If a PR contributor cannot answer questions about the recipe they are themselves authoring, and the decisions they have taken to do so - then that only delays reviews because we don't have enough information to make a call.

• It's clear that in 99% share folder contains some auxiliary files, which are not needed in runtime (and a lot of recipes remove this folder), but for some reason reviewers still ask "why share folder is removed"? If reviewer is really interested in content of share folder, he / she can build the library locally and observe everything by himself / herself. I believe recipe author does not provide review about library content, how it works or what files are required. Library public documentation can serve for these needs.

Some recipes do not work if the share folder is removed, this has happened several times. So we want to double check, especially considering how intentionally minimal the logic in the test package are. If you expect the reviewers to check out the branch, modify it to stop removing the share folder, build it locally, test - then I wouldn't be surprised if the reviews take longer.

A simple couple lines in the PR description: "share folder is removed because it only contains manpages files" or something of the sort, is all we need. Ultimately we ask PR authors to explain and justify what they intend to have merged in the repository.

---

All in all, please understand that we know CCI wouldn't be where it's today with over 1700 libraries available without its contributors, but our priority is to ensure quality over quantity, and we understand that that alongside our resources for reviewing can generate friction, so we appreciate civil discussions about how can better improve in our goals :)

Happy to talk things further

[ilya-lavrenov]

@RubenRBS I really appreciate you for your position and thank you for spending time writing this answer. Hope, other experienced maintainers also will find time to provide their opinion.

But I cannot say that I agree with you and see your answer as rather a justification why things work this (read slow) way. Let me comment partially your answers:

The recommended approach is to have your own fork of ConanCenter repository where you build and store the necessary packages for your organization

I would say it contradicts the whole principles of open source:

• Let's imagine several organizations created their own forks, where they have added their recipes and fixed issues in existing ones. Because code review in conan-center repo is slow, developers of this organization don't want to spend time with contributing changes back. Later in a half a year, it turned out that protobuf or any other dependency has a vulnerability issue. And then maintainers need to merge two codebases, and it's good if everything comes without merge conflicts. But what if is not?
• What if a lot of organizations created their own forks and refused to contribute changes back to Conan? I suppose that in this case in some common recipes such organizations will fix the same issues or create the same new recipes for required dependencies. Is it a really a goal of open source and Conan project specifically? or maybe you want people to contribute their changes back? Does not "center" in CCI name mean that it should one single center repo?
• Such advices with forks do not work for public github organizations. I can hardly imagine what Microsoft or Google have forked Conan and maintain it in their org (but even they did it, I suppose contributors will start to send changes to their repos :) )

I really wondered to hear advices to create and maintain forks from people working in open-source.

contribution in the repository is vetted before being merged

Looks like you misunderstand your jobs as "code-reviewer". You think of a contributor as a pupil, while maintainer is a teacher. But we are not in school. Your goal is not to check the correctness of every line of code in recipe, your goal is to check that recipe complies with Conan interfaces. E.g. main methods (requirements, package_info, configure, etc) / fields are used right and the content of these methods does things that seem to be true. No more.

current template in the documentation

Almost nobody uses template from documentations, people just find recipe which is similar to what they want to have, and adjust this copied recipe to new library / tool specifics.
So, if you want new recipes to be written in a good shape, change existing ones.

If a PR contributor cannot answer questions about the recipe they are themselves authoring, and the decisions they have taken to do so - then that only delays reviews because we don't have enough information to make a call.

Don't take my words out of context. I can answer the questions, but I want maintainer to think before asking such questions, because a lot of answers are already on the table. If maintainer does not know what is share folder is serving for, how those people became a maintainer? Some questions I've been asked can be googled (and my answer was just a copy from google). Again, I'm not on the school exam and don't want to explain each line.

So we want to double check

Why do you want to double check? Why don't you trust you contributors and test_package's which are part of recipe? Should I describe each line of code just because someone has concerns and wants to double check?
Again, your goal is to check that a recipe satisfies common Conan rules / API is used right, etc. Checking the content of these methods and trying to understand each line of code is NOT your responsibility. Packages managers have thousands of packages, you cannot be an expert is each package (read library / tool) and understand all specifics. You should be expert in Conan API and how to write recipes.

This is an interesting take - you are saying the first person "I" several times. Conan Center is a service that we provide to the C++ community at large - not to any particular individual

Again, you are interpreting the words wrong. "I" means not me specifically, but the use case(s) I want to enable. Definitely, these use case(s) can be a part of all use cases which this library (read recipe) can handle. And as we agreed earlier (I hope you have agreed with @SSE4 and me), that it's OK to enable subset of use cases taking into account that recipe is written in the scalable, maintainable way (and for my case this is 100% true).
Don't put it like I wrote something which works only for me.

You are asking that reviewers accept your changes according to your needs

Correct would be "I want accept my changes representing my use case(s)". All other use cases can be enabled in separate PRs.
Specifically, my use case is a TBB use case, TBB use case is use case of all TBB users on hybrid processors (all ARMs for example) (including our product).

Some recipes do not work if the share folder is removed

Why tests are needed then? To be continued in next answer..

Yes! Don't only look into this as trying to simplify for simplicity shake's, this is also a question of reducing CI workload

No! If tests are written the way they test not only include headers / library linkage, they can check more mistakes. Having 15 lines of test code instead of 10 does not mean that CI will take significantly more time to validate the recipe. I hope, every recipe author understands that they should run all the product tests as a part of test_package, but even 1-2ms tests can be written differently (read with different integration coverage).
Conda-forge, brew - all these packages managers encourage to have a minimal test checking integration aspects, e.g. that in ecosystem of that package manager and its dependencies, tested recipe / formula / feedstock works as expected.

A simple couple lines in the PR description: "share folder is removed because it only contains manpages files" or something of the sort, is all we need

Write it in contribution guidelines. If I would know, that it speed-up code review, I would write this info.
I don't see something meaningful here. What I did - opened other PRs as examples and provided description like there.

Because reviews are dismissed after every new change

This is not right. @SSE4's approve is kept even after new pushes.

we might miss it and not re-review it until we notice it again

Just subscribe to notifications properly via github notification center:
.
And click on bell to see new request on review.
But even if I ask explicitly people to review PRs, almost nobody comes.

Compilations absolutely dominate our work time for the build agents, and that we can do little about, it's the whole point of the system in the end

Right, compilation, but not the tests. Tests are usually about seconds, which is definitely not critical. But they value they bring is hard to overestimate.

not actually taken into account by the Automatic merge bot to understand when a PR is ready for merge, and the core team members, for which you need at least 2 of for a PR to be mergeable

I suppose you need to think about starting to trust community contributors and count their approves.
What is the goal of having community contributors otherwise?

our priority is to ensure quality over quantity

In my review only 1 issue has been found during review. All others are just recommendations, which spent my and your time. I suppose you overestimate the value of code-review and turn it into bureaucracy. We are not talking about surgery on human brain, we are talking about just python scripts which are 100-200 lines of code averagely.

In general, from your answers I have a perception, that you have a lot of enthusiasm (maybe you are new to open source ?) and strive to perform your job excellently and this is great, but I would do it in a different way. IMO excellent job does not mean you have to write a lot of comments to PRs annoying to contributors and showing them that you have your own opinion, it rather means:

• Need to think how to attract contributors to your project, how to minimize time to merge (and don't spend weeks or months with code review). Great package manager, it's not only a package manager, it's also a great optimized CI / CD pipelines around it (maybe it's even more important). Just compare how many PRs are merged into brew daily, and PRs to conan-center. And an argument "quality against quantity" does not work here, their quality is definitely on top.
• Remove (IMO stupid) rules about number of approves before merge. More approves => more bureaucracy and delays => less optimized CI / CD pipeline. If you trust your maintainers, then one review is enough. Even if you have ten people in review, it does not guarantee that recipe is merged without mistakes, otherwise I did not have to fix existing ones :)
• Really understand which comments are must-have, which are nice-to-have. I prefer to change request for must-have and approve with comments for nice-to-have. In later case, a PR author understands that he / she did a good job and final fix will turn this PR to be in a good shape. But endless comments with no reviewers coming back is definitely a bad strategy. In a corporate live there is a rule that email should be answered within 24h, consider something like this for code review (via properly configured github notifications)
• Perform more automation not in CI, but on conan client. E.g. some failures (comments with missed dependencies, etc) I see only after PR having been tested. In brew package manager, I can run brew audit --strict --new <name> and if it passed, it means no CI failures regarding code styles, linters, etc. Why conan tool cannot check that your favorite topics is equal to recipe name? It's a simple linting!
• Maybe you need to use github actions to close stalled PRs (e.g. 2 month or more). In this case you will always have actual PRs which you need to work with.

Thanks,
Hope alternative (outside) vision on this problem can help Conan maintainers to enhance the CI / CD process.

[AbrilRBS]

I can't comment on every point of your response right now (And will let @jcar87 answer to the points he has written), but let me touch on some of them so you can also see where we're coming from:

I would say it contradict the whole principles of open source:

My suggestion for @jiaoew1991 was from an enterprise perspective (As that's what I've assumed they find themselves into with that "we", sorry if I misread you!). For them, Conan Center Index is not a valid solution in most cases. This is something we have been recomending for a long time for them, as it's the best approach in those cases. Now, it's true that open source projects don't have that background, but that's another topic althogether.

Looks like you misunderstand your jobs as "code-reviewer" ...

This whole paragraph is (respectfully, please remember thruought our converstations that this is in no way a personal attack on you!) wrong. It has nothing to do with us teaching people how to write code, far from it. Specfically,

Your goal is not to check the correctness of every line of code in recipe

But it is! If we can't assure that recipes are good, then why would anyone ever use them? For all they know they would just all be broken. Checking recipe correctness and soundness is the whole point of the review process.

Almost nobody uses template from documentations,

I actually agree with you here, but I think that the solution goes the other way around too. Teach people to read the documentation, and make the templates more visible, explaining the users that describing and documenting their decisions in the recipes will make things much easier. BUT, I agree that there's also work to be done in improving what's already there, taking into consideration existing tradeoffs (Ever noticed how for example zlib's topics are still "bad"? We don't want to create a new recipe revision for no reason for one of the most popular recipes out there)

No! If tests are written the way they test not only include headers / library linkage, they can check more mistakes.

Again I agree that having more involved test packages can be a viable approach if given the right circumstances and it's something we have looked at in the past, and might do so again as a team to address concerns like this

Again, I'm not on the school exam and don't want to explain each line.

I'm interested to see where this point of view comes from. Noone is qualifying you nor your PR. Having a review that wants to know the reasoning behind a certain line is 1) the whole point of reviews as I have previously said, and 2) totally normal in any other repository, why would CCI be any different? (I know I do when I review changes from contributors at compiler-explorer and never in 7 years have I had this feedback from users)

Write it in contribution guidelines

Yes! As I have said, we're actively working on overhauling the contributor experience, and one of the points is to bring new guidelines to it to better reflect our views

If maintainer does not know what is share folder is serving for, how those people became a maintainer?

Please don't attack our skillset to properly do our job. All my coworkers are extremelly knowledgeable people, and if they are asking a question about something, it's totally something in their right to do, however trivial the question seems to be

Just subscribe to notifications properly via github notification center:

Please don't be so condescending to us either, doesn't feel good to be treated like this :(
If it was just as simple as "reading our notifications", wouldn't it be done like this already? You have to remember that the Conan project not only involves CCI reviews, but it's a whole ecosystem that generates hundreds of notifications each day, and inevitably some do get skipped/fall thru the cracks. Treating it as "they just don't want to review PRs" is counterproductive to the whole spirit of the project. We're the ones most invested in its success, and there's nothing we do (or do not do/stop doing) that does not have that goal in mind.

But even if I ask explicitly people to review PRs, almost nobody comes.

Sorry that this is your experience, but yes sometimes PRs are stuck in review until some of us actually can allocate the time to go thru it, and even then yours can get missed, because there are lots after/before it that deserve a review just as much

I suppose you need to think about starting to trust community contributors and count their approves.
What is the goal of having community contributors otherwise?

It's not about a matter of trust, but accountability. Community contributors are doing this out of their good heart and good will, but are in no way responsible for the repercussions that their reviews and actions in the repo might have. On the other hand, we're JFrog full time employees and have a responsibility to ensure that no breach of trust happens with any user that uses CCI. Community reviews are super useful because they are also extremelly knowledgeable, and the extra checks for recipes are invaluable, but in the end the we have the final work on PR mergeability.

In general, from your answer I have a perception, that you have a lot of enthusiasm (maybe you are new to open source ?) and strive to perform your jobs excellent and this is great

Been doing open source work for almost a decade now, but hey! thanks for noticing my enthusiasm! It's great to see that people notice that I've kept it after all this time, and great to see that my strive for doing good at whatever I'm set out to do is also visible :)

Need to think how to attract contributors to your project, how to delay time to merge (and don't spend weeks or months with code review).

As I said, general CCI contribution flows is something we're actively working on, yes

Remove (IMO stupid) rules about number of approves before merge. More approves => more bureaucracy and delays => less optimized CI / CD pipeline. If you trust your maintainers, than one review is enough.

No, this is not going to happen. Multiple reviews from core contributors will still be required

Even if you have ten people in review, it does not guarantee that recipe is merged without mistakes, otherwise I did not have to fix existing ones :)

Now imagine how it would look like if at least 2 other people didn't have the chance to go over the changes too

Really understand which comments are must-have, which are nice-to-have. I prefer to change request for must-have and approve with comments for nice-to-have. In later case, PR author understands that he / she did a good jobs and final fix will turn this PR to be in a good shape.

I agree! Having better internal guidelines is a possible course of action regarding this, and we'll make sure to at least discuss it internally

Perform more automation not in CI, but on conan client. E.g. some failures (comments with missed dependencies, etc) I see only after PR having been tested. In brew package manager, I can run brew audit --strict --new and if it passed, it means no CI failures regarding code styles, linters, etc. Why conan tool cannot check that your favorite topics is equal to recipe name? It's a simple linting!

But it can. There are docs on how to call the exact same hooks that CCI's CI will call.

Maybe you need to use github actions to close stalled PRs (e.g. 2 month or more). In this case you will always have actual PRs which you need to work with.

This is something that already exists, the stale bot removes old stale PRs for which there has been no activity in X months

[ilya-lavrenov]

@RubenRBS thanks for reading my answer.

no way a personal attack on you!

I'm totally OK with it :) I understand that different opinions / experiences may result in hot discussions and to contunie:

Please don't attack our skillset to properly do our job

Don't have a goal, sorry for that 😢

But it is! If we can't assure that recipes are good, then why would anyone ever use them? For all they know they would just all be broken

Don't assume that recipe is written by ChatGPT or contributors ententionally wrote wrong code to check whether they can pass code-review and merge non-working / vulnerable code. The recipes are written by people, tested locally and then Conan's CI is validating those recipes on multiple configurations with both build and tests correctness.
Your goal is to check that recipe (like @SSE4 wrote) "doesn't do a completely wrong thing from functional perspective". Could you please answer why do you want to understand each line of code of 1700 Conan recipes? and (hope, if code-review issue will be resolved) in thousands new ones. Do you really want to fill up your head with specifics of thousands libraries and tools?

Treating it as "they just don't want to review PRs" is counterproductive to the whole spirit of the project

Sorry, but how else can you explain that PR with 3 lines of change (from original message) was reviewed 3 weeks? Even if you are full time employee and it's your job (I understand that code review is not number one in list of your responsibilities, but growing community is extremely important to sell JFrog solutions, IMO).

Now imagine how it would look like if at least 2 other people didn't have the chance to go over the changes too

They still have a chance. Even the PR is merged, comments still can be provided and submitted as a separate PR.
Don't think that contributors want to merge their dirty code (and then go away) to your excellent codebase :) contributors also want to contribute code of acceptable quality.

In our practice we are even merging semi-working solution and then do extra work on top of it by our core team. This is because our project is significantly more complex and external contributors cannot even write complete solution without significant deep diving, because they don't know all specifics. And we encourage people for any attempt to help for the project (even via GitHub issue).

I'm interested to see where this point of view comes from

For me it's a clear analogue with school where teachers assess your homework (you have submitted PR). And then you need to perform work for your previous mistakes (apply code-review). But sometimes, when a teacher is extremely pedantic (which maybe treated as he / she wants to abuse pupils to show the power), his / her comments are not connected with reality (like cases we are trying to minimize). E.g. teacher puts 4/5 points just because you have written "Answer: 5" instead of "Answer: 5 apples"

But it can. There are docs on how to call the exact same hooks that CCI's CI will call.

Why is not this integrated into conan test?

[prince-chrismc]

👋 I really appreciate the discussion and both sides have very good points. However everything on the internet comes across as rude, and I know for a fact that is not the intent.

@ilya-lavrenov if you would like to to discuss this please email me christopherm@jfrog.com and we can setup a zoom and I can help answer your questions.

I was a conan user, CCI contributor before I joined the team and I've been with this project since it was 3 months old so I've watched it. It gives me a unique perspective :)

Thats an open offer to anyone one.

[maksim-petukhov]

Let me mention there a 3 year old PR of PCL library. Maybe this will give it a magical boost, who knows. We use unmodified recipe from this PR in production for two years and it works on Windows and Linux just OK. And nothing can be done without conan team attention because there is a problem with "infrastructure". There are also two PRs for LLVM which died because of "infrastructure" problem.

[ilya-lavrenov]

BTW, also has found a link https://www.reddit.com/r/cpp/comments/ucoyjs/conanex_conan_extended/ with comments like:

You should not pass conan-center-index PR review that could take a while and some strange changes will be requested for you to change that are not specified in docs and are known only by maintainers :)

Which highlights the problem exits (and not solved) for a while.

[db4]

Six months have gone by, but nothing has changed actually. One of the many examples: #20795. This recipe blocks a huge amount of others and was completed more than a month ago, but no member with approval rights even considered it despite numerous pings! I think you should trust your contributors: if somebody has, say, 5 recipes approved, the next ones should be merged automatically once CI becomes green.

If you care about the repo quality, IMHO the real show-stopper is constant conflicts between recipes. It is officially recommended to fork CCI and maintain your own clone for any serious use (including all the build infrastructure that one should create from scratch)! I'm afraid this is a prohibitive barrier for many potential Conan users. What I would like to see is periodic CCI releases (half a year like Ubuntu would be OK) with ALL recipes non-conflicting. With appropriate support from Conan, allowing one to choose the necessary release. This can be done with something like server-side lock files that list recipe revisions in the repo for the specified release.