Development process improvements (A New Year Wish)

[zeule]

Hi @qbittorrent/qbittorrent-frequent-contributors,

There is one thing which I don't like in qBittorrent, only the single one: qBittorrent development is slow, and not only because the team is small. Our users expect from us more than we are able to deliver right now (just look how fast the number of open issues grows). And the contributors can obviously benefit from quicker advance too. I myself find it easier to merge qBittorrent master into my own modified codebase then to merge my PRs into the qBt master branch.

Can we speed it up a bit, without breaking anything? I think we can. Here is what I propose (some items were already mentioned here or there):

1. Create a policy or a strategy for future development. In a minimal case list what can't be merged into the project and what can. For instance, simple things like "no 3rd-party libraries except those used right now (explicit list follows)", "no C++14", "no environment-specific code", "don't merge until there are open questions", etc. Publish it and keep it up to date. This will help contributors and will be needed for the next item.
2. Create an unstable development branch. Allow team members to merge PRs there after an approved review and absence of the policy violations (and GitHub can enforce the first condition). Cherry-pick or make pull request to transfer changes into the master branch. Merge master into development on a regular basis.
3. Create a bugfixes-only (or LTS) branch. Drop Qt 4 support from master already and leave it in the bugfixes branch. Cherry-pick fixes there and build Ubuntu 14.04 packages from that branch. People who use 14.04 do not need fresh software, they manifested it clearly by using that distro version. Merge bugfixing PR into that branch to continue releasing versions 3.3.x with Qt 4 for Trusty.
4. Keep dev branch (maybe master too) compatible with libtorrent master branch, but not with released versions. This will give us a way to avoid or solve situations like the current one with libtorrent 1.1 because problems then can be found by the developers earlier than users encounter them.
5. Protect master and bugfix branches and give those team members who want write permissions to manage issues (at least close old and those that are resolved already in one way or another).
6. Perhaps we can create some special branches with dedicated maintainers. For example, I would be happy to maintain a branch with Plasma-specific modifications (and anything else given it is for Linux or OS-agnostic). Maybe there are other modifications in private repos, and their authors would be happy to push it closer to upstream.

To conclude, these modifications, I believe, should make development process more dynamic and easier for maintainer(s). It is not hard to see that the project is already close to the limits of a single manpower and something has to be done to overcome this limit if we want to serve increasing demand from users and our expectations.

[sledgehammer999]

I can promise one thing. A serious problem is the ballooning of issues. Some are stale, some aren't really issues, some need reviewing, some tagging etc.

Tomorrow(or the day after), I'll give write permissions to some members in order to be able to close/open/tag issues. Some rules will apply on what can be closed or not.

PS: Write permission is required for managing issues. If you start messing with commits(pushing/rebasing/merging PRs) you will be clobbered to death with a 56k modem.
PS2: I'll try to find a way to make the other workflow a bit more streamlined.

[zeule]

@sledgehammer999 take a look at branch protection provided by GitHub (i.e. you can leave commit rights for master to yourself only).

[sledgehammer999]

@evsh link?

[zeule]

https://help.github.com/articles/configuring-protected-branches/ and links therein.

[Hrxn]

Another suggestion:
Enforce issue tracker discipline.

[sledgehammer999]

Enforce issue tracker discipline.

What is that?

[Hrxn]

Well, if you use the Submit new issue function on GitHub, you only see one unspectacular link to CONTRIBUTING.MD, which, apparently, no one bothers to read (slight exaggeration, but you know what I mean). Maybe because folks (mistakenly) believe that this is only intended for code contributions.

Until GitHub comes up with a more effective idea, one can use ISSUE_TEMPLATE.md in the repo (or alternatively .github/ISSUE_TEMPLATE.md) to display some important information in the new issue description text area. Useful for reminding new issue submitters of some necessary steps to reproduce in the description, for example, or to provide all required information before opening a new issue.

Failure to fulfill all mentioned requirements results in an immediate closing of the issue, and labeling it as "invalid" or something. And if necessary, locking the issue to prevent new comments.

[okeatime]

one can use ISSUE_TEMPLATE.md in the repo (or alternatively .github/ISSUE_TEMPLATE.md) to display some important information in the new issue description text area.

👍
We should mimic something like these:
https://github.com/arvidn/libtorrent/issues/new
https://github.com/Homebrew/homebrew-core/issues/new

[sledgehammer999]

I added @evsh @ngosang and @GotDemBallsOfSteel in the new @qbittorrent/bug-handlers team. This team has write permissions. I (think) I have protected master + all v*_*_x branches from you.

Instructions for people involved with the code that know the internals of qbt pretty well:

1. You can freely close bugs that are truly irrelevant/obsolete by current code
2. Close bugs that have a description and you cannot reproduce AND are old. Take care to not close bugs that are affecting people but you yourself cannot reproduce due to something related to your setup. For example, if users are reporting network or performance issues it isn't nice to say "can't reproduce in mine so I'll close this". I hope I haven't confused you.
3. Of course, try to triage the bugs before closing. Unless the bug is clearly fixed/obsoleted.
4. Try not to tag me for attention unless absolutely necessary. I have too many emails from github notifications already.
5. Tag bugs. Create new tags if necessary.

For non-developers:

1. Read what I wrote above.
2. Close bugs that you are sure are irrelevant/obsolete/fixed. Since you aren't a developer and you don't know qbt internals take extreme care before closing bugs that are a bit more involved.

PS: @GotDemBallsOfSteel is our forum overlord(and current site hoster), for anyone wondering.
PS2: If anyone else wants to be bug handler, leave a message. But if you don't have a previous presence here or aren't a dev I'll probably won't accept you. Also I'll try to not give to too many people bug rights, because it will create managing problems probably.

[sledgehammer999]

I forgot to mention that since something is very old doesn't mean it should be closed automatically.
Also the rules I outlined above aren't restrictive. I just give a general idea on how to behave on the bug tracker. You can do more things if you like, but try to not be overly strict.

[zeule]

@sledgehammer999, thanks, I will try to be useful with issues management, of course. Did you have chance to think about the other 5 items?

[ngosang]

Thank you!!!

[sledgehammer999]

About number 3:
Now that I merged the new icons, the next release will be 3.4.0.
I think 3.3.10 is very stable. So I'll stop packaging for Trusty. (vidid and wily seemed dropped from the repos too). So now, we can drop Qt4. AFAIK Windows + macOS qt5 is very stable.

For the other issues, I'll respond another time. I need time to think them.

[zeule]

Excellent!

[Seeker2]

Is there a consolidated list of known problems in qBitTorrent v3.3.10 (or v3.3.7-9)?
If not, maybe we should start one like there is for qBitTorrent with libtorrent v1.1.x at: #6132

[glassez]

It looks like "sole" project, and all the other contributors do not have "political weight" here. The development process is arranged here on the basis of subjective opinions of @sledgehammer999, while others often do not like it. IMO, the main disadvantages of it are the following:

1. The @sledgehammer999 desire to control every line of new code (very unproductive).
2. The @sledgehammer999 desire to keep the master branch in a stable state (really, pseudo stable) and his reluctance to have development branch.
3. The @sledgehammer999 lack of the ability to implement his desires (described above) in sufficient pace.

@sledgehammer999, do you really satisfied with the current state of affairs in the project?

[ngosang]

I don't have to much to say, but if this were my project, I would give @glassez rights to merge into master. He is an experienced programmer and knows the code quite well.

[sledgehammer999]

Yes guys, unfortunately you are right. The merging throughput is an issue. I believed I could find time to do it but always something comes up. For example, the past 3(?) weeks that I went AWOL was a real setback. If you remember, before that I had small "deluge" of mergings and I had in my mind-cache extensive code state and I would be able to quickly review the "big PRs". But after my pause that state is lost again.

In the meantime there are quite a lot of small PRs that could be relatively easily reviewed and merged with minimal risk of breaking stuff.

So I hear your objections. And let's devise a plan to go forward. I don't have solid and complete suggestions, and some things might need clarification from you too.

I propose to keep this branch structure:

1. Branch named v3_3_x etc are totally stable and contain fixes and commits pertaining to that versions number
2. Master branch should be kept in a semi-stable state. From this branch stable v branch should occur.
3. A dev branch which can be unstable and highly volatile. By volatile I mean that history rewriting is permitted. Not only squashes or deletions, but actual periodical rebases on master.

(assume that other members apart from me will have write+merge permissions)
Question is what is the relationship between master, dev and PRs?
I don't have a clear and complete vision on this. Commits should flow from dev to master. But by which criteria? I have no idea. Do you have proposals?
Under which criteria should a member merge a PR? Should we require at least 2 OK'd reviews(github feature) from project members? Should we allow merging by members only for small PRs?
Basically has anyone worked on a project with that model? Can you outline how it should work?
I understand that you want an unstable branch that you'll have to permission to merge PRs somewhat quickly. But how do we backport to master or v branches? On what criteria? I think my question/concern boils down to this: What is the difference between such a dev branch and eg me starting to merge PRs based on member reviews, without actually looking at them myself? Most of you have solid works when the PRs are small or medium. Bigger PRs are an issue, especially refactorings.

On big PRs you participate in reviewing fervently and actually discuss design choices, which is a very good thing. But from my last mergings on big PRs I had the feeling that none of you actually looked at the PR as a whole and see if something was missed from old code(in case of refactor). Or what the interaction of surrounding code is. And this can result in subtle bugs. Of course, even I can't guarantee that my review will squash all possible bugs in the PR. We can't rely on the PR author no matter how good he is. Our eyes+mind play tricks on us when we try to review our own work. That's why a 3rd party is needed to review.

Don't get me wrong. All I want to say is that big PRs need better and more thorough review as whole. I am not against allowing members to merge such PRs but we need to hold a higher standard of review for those particular PRs.

Don't forget that this client is quite popular. Don't forget there are people that have hundreds or thousands of torrents in the list. It isn't nice trashing their list/data/progress because of a quick review. It has happened in the past and it will happen in the future, but let's take some rudimentary precautions against this at least(= good reviews).

I am sorry for my mumbling, maybe my concerns aren't warranted. But be assured that now I want to give some form of merge control to people other than me.

Another last-minute proposal that I thought of.
Honestly, I don't know the level of interaction you're interested in. But in case you're not against "fixing other people's code yourself" then I can see another quick merge road. I can give merge access without fuss for the following. I think you have noticed the backlog of PRs. Most of them are useful but the authors don't respond anymore(this is on me). I had begun reviewing, resolving conflicts locally, and fixing their code to our standards and then pushing to master. Does anyone of you want to start doing that? Starting from the older PRs and working your way up. IMO, only somewhat medium-to-big PRs should need reviewing of the final code before merging. That reviewing could done by any other member apart from the one handling the PR.

Or forget the above and propose another plan entirely

[Matth7878]

I am no programmer but as a user I think it would be a good idea to propose monthly or weekly or daily build so user could choose to test development branch and report issue.
Maybe there could be a second round to make a beta from development build for further testing and to squash all remaining bug. Only then it will become master and official version would be released?

[glassez]

@ngosang, Thank you for your trust. I'm just hobbyist.
I'm not trying to achieve better rights for me personally, but for the community. We all make mistakes, we all have little time for detailed tests of our own changes... but many of the PRs here do not require the attention of the maintainer (especially if such attention is not available for weeks and months). IMO, if it is approved by several project members, that should be enough.

1. We must allow that the master branch was unstable between releases.
2. We should have several project members with write access to merge approved PRs (IMO, each project member should be able to apply some trivial changes, the correctness of which he is sure).
3. If the PR carries some "significant" changes (eg. new features or user-visible changes to existing one), it must be approved by the maintainer (in General terms - he does not have to study the entire code for this).

[glassez]

@sledgehammer999, sorry, I'm writing my comment above still not reading yours. I'll answer you later.

[sledgehammer999]

@glassez building on your #6210 (comment) (which you wrote without reading mine):

ut many of the PRs here do not require the attention of the maintainer (especially if such attention is not available for weeks and months). IMO, if it is approved by several project members, that should be enough.

Exactly! And I agree with that. (written in my previous comment.)

We should have several project members with write access to merge approved PRs (IMO, each project member should be able to apply some trivial changes, the correctness of which he is sure).

No problem here as mentioned earlier.

If the PR carries some "significant" changes (eg. new features or user-visible changes to existing one), it must be approved by the maintainer (in General terms - he does not have to study the entire code for this).

Agreed. And IMO, some big changes should at least be partly based on consensus. I think I already do that in some extent.
Building on my previous comment about "big PRs". Such PRs should outline in their first post what their trying to achieve. And maybe list caveats/compromises. I can read and approve such a post very fast. Other members will have the initial burden of reviewing. And if some reviewer explicitly states that he read it whole and put a solid effort that it is ok, the PR could be merged if I don't respond after X days/weeks.

We must allow that the master branch was unstable between releases.

I don't comment on this, since it is related on something I wrote in my previous comment. And I think it partly addresses this and partly asks questions and clarifications on how it should work.

[glassez]

@sledgehammer999, here is my plan:

1. master is main development branch. It may be unstable at some point. We merge approved PRs into it. We push some quick fixes into it directly (without creating PRs).
2. When there are enough of significant changes and new features before latest (major/minor) release we create next release branch from master. We apply only fixes and small cosmetic changes to it until next release (it can be done by apply some individual commits, or even by merging current master if it has appropriate state at this point).
3. Stable branch and master are independent. We can merge master into stable branch after the next release, if the master code conforms to the release plans at that point, or to apply only some correcting patches.
4. Stable branch doesn't mean it's stable at any point. It means we shouldn't apply big changes to it and we need to focus on increasing its code stability and fixing bugs.
5. We should produce official alpha/beta versions before big (major/minor) releases. It should be accessible via qBittorrent official site.

[sledgehammer999]

We should produce official alpha/beta versions before big (major/minor) releases. It should be accessible via qBittorrent official site.

(I am commenting on this first and quickly to prevent derailment of the thread. I'll comment on the other points later.)
As long as the PR review pressure is relieved from me I believe I can manage such release plans. In fact, I might be able to adhere to a pre-planned release schedule.

[glassez]

Don't forget that this client is quite popular. Don't forget there are people that have hundreds or thousands of torrents in the list. It isn't nice trashing their list/data/progress because of a quick review. It has happened in the past and it will happen in the future, but let's take some rudimentary precautions against this at least(= good reviews).

Agree.
I do not propose to produce unstable releases! On the contrary - I just want you to admit that our master is unstable even now, when you're trying to keep it stable (It's not working. Statistics collected by me over several years, says that no matter how carefully we studied some PR, errors detected for several hours/days after it was merged into master and other developers began to use this new code). And, recognizing that, separate it from real stable branches.

some big changes should at least be partly based on consensus. I think I already do that in some extent.Building on my previous comment about "big PRs". Such PRs should outline in their first post what their trying to achieve. And maybe list caveats/compromises. I can read and approve such a post very fast. Other members will have the initial burden of reviewing. And if some reviewer explicitly states that he read it whole and put a solid effort that it is ok, the PR could be merged if I don't respond after X days/weeks.

Agree.

[glassez]

In fact, I might be able to adhere to a pre-planned release schedule.

I do not mean that we must strictly adhere to any plan (we have to admit that we can't always implement some kind of planned feature in a reasonable time). I just meant if after the release the master still contains no invalid for stable branch changes, we can just merge it there.

[zeule]

While supporting all the main ideas of this thread (a development branch, stable branches for releases or development branch and stable master), I don't understand why history rewriting in the development branch is needed.

Should we require at least 2 OK'd reviews(github feature) from project members?

+1, I hope we have enough active people to get those two approvements.

And IMO, some big changes should at least be partly based on consensus.

I would try this for all PRs, or at least allow vetoing in some form.

some big changes should at least be partly based on consensus. I think I already do that in some extent.Buildng on my previous comment about "big PRs". Such PRs should outline in their first post what their trying to achieve. And maybe list caveats/compromises. I can read and approve such a post very fast. Other members will have the initial burden of reviewing. And if some reviewer explicitly states that he read it whole and put a solid effort that it is ok, the PR could be merged if I don't respond after X days/weeks.

+1

I do like dev + stable + release branch scheme (for example, https://wiki.qt.io/Branch_Guidelines). Please note, that master branch is the default branch and it is this branch most new users will face. As such there is a reason to keep it stable (that is almost a quote from the Qt Guidelines).

We should produce official alpha/beta versions before big (major/minor) releases. It should be accessible via qBittorrent official site.

+1

In fact, I might be able to adhere to a pre-planned release schedule.

I do not mean that we must strictly adhere to any plan (we have to admit that we can't always implement some kind of planned feature in a reasonable time).

We may do time-planned releases, giving the stable branch a few weeks to mature and increasing appropriate version number basing on the feature set of the new release. If we solve our workflow bottleneck problem, these releases will contain bugfixes anyway.

[glassez]

I don't understand why history rewriting in the development branch is needed.

+1

We may do time-planned releases, giving the stable branch a few weeks to mature and increasing appropriate version number basing on the feature set of the new release. If we solve our workflow bottleneck problem, these releases will contain bugfixes anyway.

Reasonable. But we should be able to deviate from the schedule, for example, for hotfix releases, etc.

[glassez]

Please note, that master branch is the default branch and it is this branch most new users will face. As such there is a reason to keep it stable (that is almost a quote from the Qt Guidelines).

IIRC, Qt just makes the current stable branch to be the default branch. It has no separate default branch. We can do the same.

[zeule]

changes for one-man-maintained features do not affecting other parts of the application (e.g. @evsh may push commits for Cmake building system).

+1, what about OS-specific changes? For instance, should I want to change .desktop file name (#6625) or unbundle icons for Linux installs?

[sledgehammer999]

Yes minor change like typo fixes, you forgot something or something really benign can go straight to master without PR.
Also CMake changes can go too, unless you restructure a lot of source files which will create conflicts with pending PRs. (you might want to open a PR anyway to test via travis/appveyor that your changes actually work across platforms)

should I want to change .desktop file name (#6625)

I think this one is ok to go straight to master but

unbundle icons for Linux installs?

IMO, this needs PR for discussion and review of possible side effects.

[zeule]

OK, let's rely on common sense then. Let's see how that goes.

[Chocobo1]

Maybe somewhat related to this topic:
Can we tag some issue tickets for hard blocking the next release?
Some issues related to basic usage (exclude the insanely hard to fix ones) should be take care of, specifically: #6454, #5503 (comment)

shipping with those defects is gonna piss users off...

[zeule]

GitHub provides us with milestones. Will that do the trick?

[Chocobo1]

Will that do the trick?

yeah if we use it properly.

[sledgehammer999]

Now, I think the 3 of you have proper rights for PRs and commits to master and v3_3_x. (And I think you have access to create new branches and handle bugs).

Can we tag some issue tickets for hard blocking the next release?

Milestone can be used to collect desired goals. And a bug label can be created to attach to bugs to denote hard blocks. Of course, if noone is looking at them or trying to implement/fix, they will transition to "wishful thinking" :p

[glassez]

@sledgehammer999, IMO, it would be better having more official team names (unlike demigods, e.g). Also qbittorrent-frequent-contributors has redundant part in it (qbittorrent), it is repeated twice, when we mentioned this team (@qbittorrent/qbittorrent-frequent-contributors).

[sledgehammer999]

What do you propose in place of demigods?
PS: If you don't know already, after you hit the @ symbol you can start writing the team's name without mentioning the qbittorrent/ prefix.

[zeule]

Now, I think the 3 of you have proper rights for PRs and commits to master and v3_3_x.

Thanks! Settings seem to be in place: GitHub allows me to merge into master.

[zeule]

Tried to push cmake stuff to master, but got this reply:

remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: At least one approved review is required
To github.com:qbittorrent/qBittorrent.git
 ! [remote rejected]     HEAD -> master (protected branch hook declined)
error: failed to push some refs to '[email protected]:qbittorrent/qBittorrent.git'

[zeule]

Hi @qbittorrent/demigods,

Since we don't have a policy how PR should be extended or corrected during a review process (some amend, some add new commits) in the case of amends it is sometimes hard to understand what changes you approved already (yes, #5532, I'm looking at you) and, what is more important, notice that there are new significant ones. And while I encourage everyone to add new changes via new commits with --fixup or --squash options, there is a setting in GitHub to dismiss an approval when new commits are added. Perhaps, it might make sense to turn it on?

[glassez]

there is a setting in GitHub to dismiss an approval when new commits are added. Perhaps, it might make sense to turn it on?

+1

[glassez]

What do you propose in place of demigods?

Maybe developers (or even devs)?

[Chocobo1]

there is a setting in GitHub to dismiss an approval when new commits are added. Perhaps, it might make sense to turn it on?

+1

Maybe developers (or even devs)?

+1 for devs

[briankendall]

(yes, #5532, I'm looking at you)

Personally I don't like to keep squashing commits, and in my own repos I prefer PR's not to get squashed . In fact it's been hard for me to keep track of changes in my own branch. I was asked to squash things down originally when my PR wasn't as big as it is now, and figured I needed to keep doing that. I think having a clear policy on when to squash the commits of a PR and when to keep each individual commits would be a good idea.

[glassez]

I think having a clear policy on when to squash the commits of a PR and when to keep each individual commits would be a good idea

IMO, the policy is clear. All commits that fix some other commits from same PR should be squashed before merging.

[briankendall]

@glassez Is this to say then that all PR's should be squashed to one commit, or that each major change in a PR should have its own commit, and any fixes should be squashed into the appropriate commit?

[glassez]

each major change in a PR should have its own commit, and any fixes should be squashed into the appropriate commit

Yes. For example, in your PR (mentioned above) you can have one commit that changes AddTorrentParams by adding firstLastPiecePriority and TorrentHandle to accept changed params, the second one that changes AddTorrentDialog to accept AddTorrentParams, and third commit that add new command line parameters handling (because they are relatively independent changes).

[ngosang]

@qbittorrent/demigods @sledgehammer999 It seems like things are getting better quickly. Thank you for your hard work. 😄

[glassez]

If you need my attention for feature approval you should leave a comment with this string somewhere in it: @sledgehammer999 approveFeature

@sledgehammer999, isn't GitHub approval request enough for this purpose?

[sledgehammer999]

@sledgehammer999, isn't GitHub approval request enough for this purpose?

I am not sure I have a way to setup an email filter in that case.

[sledgehammer999]

FYI, I have enable the setting to require approvals again after new pushes to the a PR a few days ago.
I haven't changed the team name because there is an autogenerated team from github that is contains the admins of the project. It is a "legacy" team, it lets me delete it, but first I wanted to make sure I don't break something in the admin rights. When I find time I'll do it.

[ngosang]

@zeule close?