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

conan-center-index official community reviewers #2857

Open
memsharded opened this issue Sep 8, 2020 · 18 comments
Open

conan-center-index official community reviewers #2857

memsharded opened this issue Sep 8, 2020 · 18 comments

Comments

@memsharded
Copy link
Member

memsharded commented Sep 8, 2020

Pull requests opened to this repository, conan-center-index are reviewed by the maintainers and the community. To be merged, every pull request should have at least 3 positive reviews, at least one of them from one of the maintainers, but the other can be from any of the approved contributors:

@madebr
@SpaceIm
@ericLemanissier
@prince-chrismc
@Croydon
@intelligide
@theirix
@gocarlos

Many users have been doing great contributions to ConanCenter, and we are really grateful for those amazing contributions, thanks very much to all of you! These users have been enabled due to their outstanding history of contributions, with pull requests or reviewing and helping other users. This list might evolve and new contributors might be enabled in the future as well. Please read conan-io/conan-io.github.io/pull/145 for more details.

Note: The feature is not enabled yet, we will update as soon as the system enables it.

If you have any questions, please do not hesitate to contact us.

@ericLemanissier
Copy link
Contributor

Thanks! In order to help us (actually me) be up to the task (as the quote says: 🕷️"With great power comes great responsibility" 🕷️), is there already a set of guidelines for code review on CCI ?

@memsharded
Copy link
Member Author

Thanks! In order to help us (actually me) be up to the task (as the quote says: 🕷️"With great power comes great responsibility" 🕷️), is there already a set of guidelines for code review on CCI ?

Good quote :)

We don't have any explicit review guidelines yet, in general we try to convert those guidelines to hooks whenever possible, but this is a good suggestion. I would say to apply your knowledge and common sense, raise any possible question you might have here, as an issue (maybe create a new tag or [reviewers] in the title), and we can start adding those answers in a dedicated section in the docs?

@uilianries
Copy link
Member

A new file in conan-center-index/docs would be nice I think. All centralized and corresponding to CCI.

@danimtb
Copy link
Member

danimtb commented Sep 18, 2020

With the latest changes in the build service https://github.com/conan-io/conan-center-index/blob/master/docs/changelog.md#17-september-2020---1742-cest we are now able to merge PRs automatically.

Here is an update on the current state: #2899 (comment)

And we will enable 2 community reviews starting next week! 😄

@prince-chrismc
Copy link
Contributor

To help close some open PRs which are ready, I wanted to share my search filters.

Currently uneventful.... but quick and easy

For those pesky PRs we'd rather forget

@danimtb
Copy link
Member

danimtb commented Sep 21, 2020

Finally, we have enabled the announced configuration.

Staring today, PRs will be automatically merged requiring just one approval from the Conan team and 2 additional ones coming from the above-mentioned community reviewers. Only the Conan team members are allowed to "block" a PR by requesting changes for now, but this can be changed in the future as we test the new feature.

I would like to document this community reviewers list, with the details about how the automatic merge works as well as the useful links provided by @prince-chrismc, so I will keep the issue open until then.

Thanks a lot for all the effort on the PRs, the reviews and reports that keep improving ConanCenter!! 😄 🏆 🥇

@ericLemanissier
Copy link
Contributor

Does the merging bot merge PRS based only on the number of reviews, or does it wait for successful CI?
eg. #2984 is it "risky" to approve a change before CI is finished ?

@danimtb
Copy link
Member

danimtb commented Sep 25, 2020

@ericLemanissier don't worry, it also checks that the CI is green, so you can approve a PR before the CI ends and if all the reviews are done, the bot will merge it

@prince-chrismc
Copy link
Contributor

If anyone wants a head start on reviewing PRs Monday morning, check this out!

prince-chrismc/conan-center-index-pending-review#1

Part of my learn something every weekend during lockdown 🔒 ⬇️ Feed back is welcomed!

And of course 🤗 thanks for the inspiration bot makers!

@danimtb
Copy link
Member

danimtb commented Feb 5, 2021

I think we should add a section to the documentation when we talk about the reviewers and how are PRs merged. Where do you think we should add that section?

@prince-chrismc
Copy link
Contributor

🤔 There's a few places that seem to related to the topic...

  1. docs/contributing.md#dev-flow--pull-requests
  2. docs/how_to_add_packages.md
  3. 🆕 docs/review_process.md (link from dev flow)

For bots, it would be really nice to have a seperate file with the the community reviewers 🙏

@danimtb
Copy link
Member

danimtb commented Feb 18, 2021

Glad to announce that we are adding one more community reviewer to conan-center-index! Welcome @mathbunnyru and thanks a lot for the hard work. See #4608

@ericLemanissier
Copy link
Contributor

ericLemanissier commented Feb 18, 2021

congrats @mathbunnyru, slayer of the hooks errors !

@mathbunnyru
Copy link
Contributor

Hooray 🎉

@danimtb
Copy link
Member

danimtb commented May 26, 2021

Another great addition to the reviewers' team! Welcome @ericriff as new community reviewer. Thanks a lot for the contributions and comments. See #5637 🥳

prince-chrismc added a commit to prince-chrismc/conan-center-index-pending-review that referenced this issue May 26, 2021
@zuut
Copy link

zuut commented Jun 14, 2021

First, I wanted to say thank you for setting up and running conan center. If I understand the purpose, the goal for this is to be a centralized repository of conan recipes for all c++ open source projects, allowing anyone to easily find the correct conan recipe for a package. With that in mind, I have a few comments that I hope are helpful and constructive as I would like for this to become a reality given that c++ has lacked such a mechanism that is found in languages like rust, java, python etc.

1.Please provide a rich cmake recipe that installs binaries and libraries in a manner that you'd like others to emulate and update your guidelines to indicate that particular recipe. I'd also do this for other types as well. On many occasions, the feedback in the review comments stated that things are done incorrectly in spite of being done in the same way in existing recipes already in conan-center repo. In fact, they were done in the "recommended good recipes" listed in conan-center's doc . This could be avoided if the example recipe in the guideline did it the way many reviewers believed to be the correct way.
2. I also had issues building on the macos versions but there wasn't a way for me to debug what was happening. It turns out after many attempts of changing the recipe to get more info about what was wrong, that the c pre-processor on some of the mac os build servers wasn't found at the location /usr/lib/cpp. Its there on my own macos systems, but apparently missing on SOME of your build servers. I understand not having access to the build servers to debug the issue but perhaps this could be avoided by having a standard vm or system configuration published so that we can debug the issue on our own hardware.
3. Create a checklist of things to look for and publish that checklist so that both reviewers and submitters can follow it. IMHO, reviewers should be checking whether a recipe follows the guidelines and NOT whether it follows the reviewers own personal ideas of how things should be done.
4. So far this process has been going on for 2+ weeks and doesn't appear to be in any near an end in sight. If you truly want to open up conan-center as the place to go, you should think about speeding up this process.

Again, thank you for doing this and I truly want this to succeed but in my experience its a bit too arduous a process if you want widespread participation.
Cheers!

@intelligide
Copy link
Contributor

@zuut First of all, this issue is not the proper place for this discussion. Please open a new issue to discuss your views and the improvements you think are important to make to the CCI. 😉

For the point 1, Many of the older recipes are not updated and use old, non-recommended practices. The CCI is now large and contains many recipes, so it would normally take a lot of time to check and modify each recipe whenever a new good practice is added (which would further hinder the addition of new recipes or versions).

For the point 2, You can test your Linux package with Conan Docker images. For some legal reasons, Conan can't distribute macOS and Windows VM/images. If you need special software in the build machines, open a new issue.

For the point 3, Take a look at the docs for some best practices and rules. However, we are sometimes confronted with situations that do not fit in the rules and we are open to discussion to find the best way to handle the exceptions.

For the point 4, we all know that the process takes a lot of time, even if it has been greatly reduced thanks to the community reviewers (of which I am one). However, the validation time is really variable. Recipes that are simple or do not require many feedbacks/changes are accepted very quickly, as well as updates that do not require major modifications. Newcomers are usually faced with large validation periods to simplify the proposed recipes. Conan is a tool that adds a lot of features with each major release and the CCI uses them to ensure the best possible maintainability for the packages, this means that newcomers are not necessarily familiar with all the tools and practices in the CCI. Finally, reviewers are volunteers who have a job and cannot focus full-time on the CCI.

Thanks. 😄

@danimtb
Copy link
Member

danimtb commented Apr 21, 2022

As per changes noted at #10417, we added more people to the community reviewers list! Welcome @AndreyMlashkin @MartinDelille @dmn-star!

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

No branches or pull requests

8 participants