PR | Testing & feedbacks | Waiting to be merged |
#1335 (https://github.com/SFML/SFML/pull/1335) | 2 months | 2 days |
#1371 (https://github.com/SFML/SFML/pull/1371) | 12 days | 7 days |
#1361 (https://github.com/SFML/SFML/pull/1361) | 2 days | 10 days |
#1334 (https://github.com/SFML/SFML/pull/1334) | 3 weeks | 7 days |
#1377 (https://github.com/SFML/SFML/pull/1377) | ~10 days* | 7 days* |
#1395 (https://github.com/SFML/SFML/pull/1395) | 2 days | 4 days |
#1384 (https://github.com/SFML/SFML/pull/1384) | 4 days | 3 days |
#1356 (https://github.com/SFML/SFML/pull/1356) | 2 weeks | 1 day |
#1350 (https://github.com/SFML/SFML/pull/1350) | 2 weeks | 0 day |
#1345 (https://github.com/SFML/SFML/pull/1345) | 2 days | 7 days |
#1342 (https://github.com/SFML/SFML/pull/1342) | 7 days | 7 days |
Ultimately my feelings is that the whole team wants to ensure SFML keeps a high code quality standard. It's part of its 'personality'. That's why I suggest we request the final approval by a team member. My hope is that with more feedback this will go much faster as grey area in the code would already have been discussed.Agreed.
Your suggestion for adding a tag and putting more light on those blocked PR is a good one, I think.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.
I've tried promoting a few of my PRs on the forun a while back. Sadly, it didn't work. It also adds some work.Was it mixed with other topics or in a dedicated section? To me it’s really important that people who help by reviewing or testing can do it without searching for these topics. There should be list that shows where help is wanted. Topics in a distinct section is one possible way to expose this list.
Maybe we should instead regularly advertise that people can subscribe to github issues if they want to contribute. But in any case, people can always open a thread for a specific PR and attract the attention of the community.I'm afraid we won't get results if we can't make this become a habit, I mean the people who'd like to help would maybe not get used to help if a topic requesting help is created only sometimes. You could think of this as virtually creating an easy workflow for people who want to help.
Currently merging is done in batches to make rebating less tedious. If we merge a PR everyday or so, every other PRs need to be updated. So we have to find a tradeoff."rebating"?
From my outside view visibilty is a problem and the process is not so clear at all. There are SO MANY issues in the tracker!Not willing to upset you but the PR process is already big enough to deal with, so let's remain focused :) If you want to talk about GitHub issues you should open a distinct thread.
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 :)I agree eXpl0it3r is doing an amazing job. But if PRs wait a full week to be merged there's still something wrong. For now I still don't know if the delay if because of batching or because the free time of a single person is not enough so will wait for eXpl0it3r's answer on this matter.
2. Reviewing and testing needs time, I don't think there's any good reason for rushing to merge a PR as long as it is still active. The problem, in my opinion, is when PRs get stuck (or worse: forgotten) because there's suddenly no one to test or review it.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.
3. We've already tried various strategies for getting more people involved in the review & testing process, but they all failed. The truth is that only the author of the PR and, with some luck, one or two SFML team member, really cares to actively work on PRs. Others just wait until it's done and available.Can you give more details about what you tried?
4. Unlike you, I think we're doing a really great job at handling issues and PRs. Issues and PRs are submitted quite frequently, and so far we've been able to handle them, allowing SFML to remain active and evolve even without the team members working on it. Compare the situation to what it was before: I think we've made huge progress.I'm not saying you're not doing a great job. You do a great work with current team. But that doesn't mean it (the process, not your work ;D ) is satisfying from PR authors point of view, because of the delays mentioned above. I don't know how the situation previously was so I believe you about the progress.
So the only problem is the lack of testing & review from the community, and in my opinion this cannot be solved. People who want to help already do it. There's nothing we can do to force others to get involved.Waiting for your answer on 3. before answering this. But yes there are things to do, and you can't force people to get involved, but you can motivate them.
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/2
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.
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. ;)
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.
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 :)
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.
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?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.
[...]
When testing is needed, can it be done by anyone or should there be at least 1 tester from SFML core team?
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. ;)
[...] having CI also check the code after merging changes into master would help identify issues. I think it should be possible to configure a bot to report simple but helpful instructions for contributors (e.g. Please rebase your code).A reminder is still an improvement but a small one. What would really help would be the automatic test with merge. I guess it depends on the CI itself, so the first step would be checking whether the currently used CI system does support this. If we have to implement it ourselves it's not worth it to me.
Of course, if we agree on such changes in the infrastructure we would need people willing to help implement those changes. ;)
Also, what would be the added value for contributors? It's just a tag, helpful for searching, but not influencing the discussion happening in the PR.That's the point, easier searching. But no need to care about it if we go with forum solution or if we can get people used to the GitHub Project page.
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.For sure we don't have the resources at the moment (or at least we don't see them) but I disagree when you say it's not realistic. Of course this is subjective so let's move on.
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/2I've seen it but I don't consider it to be a usable tool unless any PR author can change it, which I guess you don't want so that you can keep control. Probably a matter of trust. Again the purpose is to make the list of PRs ready for review/testing visible immediately, not to depend on someone from the core team for that.
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.Could you give a ratio of the PRs where someone has actually raised a concern during this "Ready" stage? On all the PRs I've looked at I don't remember any where there was a concern, but maybe I just didn't take a representative sample.
What's the reason that "merging as fast as possible" is so important to you?So as I already stated
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.Additionally here are a few other reasons:
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.Well if it's an issue for any/some contributors, then this is your issue. Because without contributions SFML dies. I'm just one contributor among many others, but I don't think telling "this is problem with you only" is gonna help with motivation and contributions :) .
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.Of course reviewing and testing takes time. But it for sure shouldn't need 2 months.
In my books 2 months of testing and feedback is quite realistic for the given change set.
Interesting to know!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.
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.Ok.
As usual, the more help the better!
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, [...]What's sure is that not changing anything won't make more people come help :P
Things we already have setupFor curiosity, do you know if anyone is using the GitHub project apart from you? (NB: I'm not implying it doesn't make it useful, even if you were the only one using it)
- GitHub tags & milestone (https://github.com/SFML/SFML/milestone/14)
- GitHub project (https://github.com/SFML/SFML/projects/2)
- CI artifacts (binaries) for each PR (https://artifacts.sfml-dev.org/by-branch/)
- How to help test thread (https://en.sfml-dev.org/forums/index.php?topic=17798.0)
- Contribution guidelines (https://www.sfml-dev.org/contribute.php)
I'm not sure I follow. What I meant was that our CI would, in addition the existing tests, test the code after a local merge of the branch into master and, if any of the tests fail, the CI bot could emit a message on github with the error details.Quotehaving CI also check the code after merging changes into master would help identify issues. I think it should be possible to configure a bot to report simple but helpful instructions for contributors (e.g. Please rebase your code).A reminder is still an improvement but a small one. What would really help would be the automatic test with merge.
I've seen it but I don't consider it to be a usable tool unless any PR author can change it, which I guess you don't want so that you can keep control. Probably a matter of trust.It's not really a matter of trust, although if every kiddy can mess around our boards it would be a mess, obviously. But I think it absolutely makes sense to keep this board in the hands of a few. Why? Because, like a real company, not all developers are project managers and those are the only one that should organise priorities, IMO. The same applies, less critically, to tags in the end.
For curiosity, do you know if anyone is using the GitHub project apart from [eXpl0it3r]?I do go and have a look from time to time, yes. Gives you a good overview of the progress and current status.
I'm not sure I follow. What I meant was that our CI would, in addition the existing tests, test the code after a local merge of the branch into master and, if any of the tests fail, the CI bot could emit a message on github with the error details.I misunderstood, what you describe looks nice :)
Did you mean that the CI should automatically merge things? That'd be inadequate for the simple fact that the tests currently consist only in compiling the code. This won't indicate whether a bug was fixed or a feature correctly implemented.No, I didn't intend to go that far.
It's not really a matter of trust, although if every kiddy can mess around our boards it would be a mess, obviously. But I think it absolutely makes sense to keep this board in the hands of a few. Why? Because, like a real company, not all developers are project managers and those are the only one that should organise priorities, IMO. The same applies, less critically, to tags in the end.Well the developers could update the board to update status about what's ready to be reviewed, etc. Without changing priorities. Anyway it doesn't look like it could be used efficiently by PR authors to give visibility about PR needing testing/review so let's forget this.
[...] a couple of quick thoughts: [...]I don't have any opinion on your ideas but thanks for the input!
I've already spent multiple hours writing my original responseAnd you may save weeks of delays :) Thank you for opening the section and I hope it'll prove useful.
and honestly, I rather invest my time into other things, than having everything I wrote being slammed down and things I suggest as improvements not being discussed at all.Well as you didn't look convinced on the "why", it's expected that there were not many answers on your "how". I didn't mean to ignore your efforts in any way.
If anyone feels like the sub-forum needs a bit more explanation feel free to create a thread and I'll sticky it. :)Right now I would suggest small additions to current Contribution Guidelines (https://www.sfml-dev.org/contribute.php) to make is somewhat become a habbit:
Additionally, I've started a PR for Issue and PR templates (https://github.com/SFML/SFML/pull/1403). I think they can still be improved, but I'm also highly interested in what others would put there. So please head over to GitHub and make some suggestions.Nice! Seems like a nice lowcost addition.
Hiura suggested to create a GitHub Project at the organization level (https://github.com/orgs/SFML/projects/1). As it is right now, I don't really see a huge benefit to it - the only difference is that we now have some website tasks there as well - and as down side, it could be less discovorable. The main SFML repo will be the entrypoint to SFML on GitHub for most and very few will navigate to the organization level to find the GitHub project there.I agree about SFML repo being the main entry point so I don't think it's needed.