I'm happy to bring some changes to our process in order to move things forward, however I think we should keep realistic goals.
The goal would be for any PR that don't need updates (ie. only review and testing time) to be merged within at most 2 days. This is of course not doable if parts of the process can be done by too few people, but totally doable if the process is scalable.
This is a very unrealistic goal, as we simply won't have the resources available right within that time frame to do in-depth reviews and testing on 3-5 different platforms.
In both cases my personal feeling is that currently the PRs that are blocked by a (code) review are not visible in SFML's PR list. The tags in https://github.com/SFML/SFML/pulls don't look to give this information.
To me it seems you're not aware of the GitHub Project (sort of Kanban board) that we (or mostly I) maintain:
https://github.com/SFML/SFML/projects/2Tracking the status of a PR or issue isn't always trivial. We have tried different labels in the past, but the issue list simply isn't very flexible. Ever since GitHub has introduced Projects, I've be using it extensively and it's a lot easier to track what is going on. There might still be room for improving the process and I might need to be more strict with moving cards around, but the important sections are "Ready" and "Review & Testing". As soon as a PR hits the "Ready" section it will be merged the next time I get around to merging things, unless someone in the meantime raises some concern and "Review & Testing" PRs require additional community attention.
Nobody talked of rushing only being more efficient to reduce the time between when the PR is opened and when it is merged. Which most likely implies sharing more work with the community.
What's the reason that "merging as fast as possible" is so important to you? When a PR is merged really isn't important, important is that reviews and testing has been done and there's a high enough confidence in the code changes.
Take your PR as an example, it's been merged now for a few days, what if the PR got merged just today, where's the difference and how is it so much more important to have it merged right the instance when reviews and tests are done?
As mentioned when something is moved to "Ready" it is as good as merged, but it also still leaves the PR open for a while, so people can do their final testing or raising some concerns. To me there's no real difference in progress tracking whether something is merged or in the Ready state.
But let's take my last PR as an example (although it's an extreme case) for which testing took 2 months, where probably 90% of that time is waiting for feedback. During that time I refrained from helping on other matters, because I didn't want to pile unmerged work up, I wasn't sure it would be merged at all, and it was rather demotivating to get sparse feedback. So what you seem to consider a good enough process means that I could have contributed more or less 3 times more to SFML in the same time range. Up to you to consider if it's worth it.
If this is reason you want to get stuff merged faster, then that's really just an issue on your end. If you don't want to deal with multiple PRs at once, then that's fine, but you can't just put the blame on the process. You could have contributed more or less 3 times more to SFML, if you used the waiting time to do it. An open PR didn't physically block you from contributing more.
As for feedback, it just takes time, especially with such a large change. There aren't hundred of people just waiting to test and review and of the people that are there to help, only a handful will be familiar enough with CMake to give this a fair try. Then you have Windows, Linux, macOS, iOS and Android that these change affects in some way. Not everyone has access to devices that run these OS or have the knowledge to operate them properly.
In my books 2 months of testing and feedback is quite realistic for the given change set.
I really insist about the fact that the PR creator must be able to set the tags himself (if we go for the tags solution). It’s important that there is no need to wait for anyone from the core team to set the tag, and that we don’t wait for the PR to look blocked : the tag should be set as soon as the PR can be reviewed.
To my knowledge this isn't possible with GitHub.
I noticed that only eXpl0it3r is merging the PRs. Why? I would expect that at least all members from the SFML core team could do this. Also I wonder if there is currently a minimal wait time before merge (I remember there were notices like "If nobody is against this PR withing X days it will get merged" in the past).
As mentioned, merging is not a high priority task, as such there's no need to spread the responsibility further. Involving more people simply increases the overhead of having to align with each other, running into conflicts or someone forgetting a step in the process.
The times given were never really accurate and with the GitHub Project, this is now more or less implicit. It moves to Ready and within the next few days it will eventually be merged down.
1. eXpl0it3r is amazing at managing tasks, I wouldn't say he is the bottleneck, and I think having more people involved in this process would just make it a big mess -- unless eXpl0it3r is fed up and wants some help, of course
It's true that we have relied solely on eXpl0it3r for the merging procedure, and that for quite a while. He's doing a fantastic job for sure. But maybe, eXpl0it3r, do you want other teamates to help?
Awww thanks
I'm good so far and everyone from the team is free to prioritize issues they deem important or want to work on. I'll in the end make sure, everything is setup in a consistent way. And if I see community members getting interested in a topic, that will also be sorted in.
Currently merging is done in batches to make rebasing less tedious. If we merge a PR everyday or so, every other PRs need to be updated. So we have to find a tradeoff.
I think the rebasing isn't really needed as long as there are no merge conflicts, it's just that GitHub (and pretty nobody else where) supports a rebase-merge strategy like we do, as such if branches are rebased and merged, GitHub won't realize that the PR was merged, so I have to manually close it. But that's no real issue and I think, we shouldn't tell people to rebase, as it just annoys contributors and doesn't really add value.
The batching is less of a rebase issue and more of, when I sit down and actually merge stuff.
As for resolving merge conflicts, it often is the easiest if the original author does it, as they know what code they added and thus will most likely do fewer errors, but if there's no activity from the author, we can also do it.
To "fix" this we would need an easy way to see the list of PRs that are waiting for review. [...] or a forum section with a topic per PR needing review. This last solution looks maybe overkill but the advantage is that it's visible from all SFML forum members that would visit this section, and if people want to help on SFML development by reviewing, they can know where to help in an easy way. If we go this way it should be the PR's author's responsibility to open the topic in the forum section, so that it's not more burden for SFML core team.
We actually had an internal discussion about this many moons ago and sort of agreed to do it, but then never actually did it.
Should the (code) review be done always by at least a SFML core team member, or can it be done by any member?
[...]
When testing is needed, can it be done by anyone or should there be at least 1 tester from SFML core team?
We depend on the community to review and test. So far I have seen about 3 "GitHub" reviews, i.e. where someone started the GitHub review process. Would be nice if this number increases, even though we also do count "looks good" to some extend.
We absolutely depend on the community to test. Everyone has different hardware and many things just need that diversity.
I don't think we can have a fixed number on who has to look at what, because it really depends. For a minor change for example, it really doesn't matter if someone from the community tests or reviews this, but when you get API changing stuff, we really do want SFML Team members to sign-off on that.
As usual, the more help the better!
ConclusionWe need to keep it realistic and aim for time frames that make sense, which IMHO is very dependent on the task and the availability of people. Plus we need to consider that simply changing the process doesn't guarantee in any way, that suddenly more people will actually spent the time to review and test pull requests, but I think a bump of one or two people occasionally is already worth the investment, plus it might give a better visibility on what is going on with SFML's development.
Things we already have setup
Things that I think could be useful
- Dedicated sub-forum for pull requests
- GitHub issue/pr templates
- Always providing proper testing instructions at best with example code
Other things that could potentially be done
- Simple document on what the process roughly looks like
- Bot to post links to artifacts, so people are more aware
- More/different tags on issues and PRs
- ...
Thanks for all the feedback and work, let's look forward to when PRs move forward faster!