Integration speed on PRs

Programmers discuss here anything related to FreeOrion programming. Primarily for the developers to discuss.

Moderator: Committer

Post Reply
Message
Author
User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Integration speed on PRs

#1 Post by adrian_broher »

I'm wondering how the workflow regarding PR integration could be improved.
  • Why does it take so long to merge PRs?
  • What is the current workflow to merge PRs?
Currently I have a significant backlog on unmerged local branches and I would like to get rid of those.
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13586
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Integration speed on PRs

#2 Post by Geoff the Medio »

What is your expectation for responses to and merging of pull requests?

If the github filtering is showing me what I want, you currently have 6 open pull requests for freeorion, and one for the SDK.

The SDK one has a comment that says it's not done yet.

Three of the freeorion ones ("String table tool", "shared library build with with CMake on Windows", and "Fix #785") are labelled as and appear to still be works in progress (like the vast majority of the old pull requests that haven't been merged). I assumed no response is needed on those in terms of review and merging until you comment that they are ready or otherwise need feedback.

The other freeorion ones, "Move SDLGUI into FreeOrion", "Replace XMLElement::NoSuchChild", and "Replace boost::bind" were updated 2 days, 5+ hours, and 2+ hours ago. The bind one already had feedback and was just updated in response.

This evening, I checked out and tested the bind one again, and was about to look over it a bit more. The SDLGUI one has me a bit hesitant (perhaps unreasonably) as it seems a bit more likely to lead to issues with the forthcoming release, and less likely to block everything else you might be working on, so I put it off a bit. The XML one looks simple, but I wanted to test it as well.

I have limited hours during a day for freeorion stuff, and I don't want to drop everything else whenever there is a reviewable pull request ready, so when your level of productivity is as high as it has been recently, particularly in terms of lines modified, there will be some delay in getting a review. Unfortunately not many other people are able and willing to review pull requests currently.

The workflow is to look over the changes of open pull requests, decide if it should be pulled and tested locally, comment / discuss, wait for updates, merge when the discussion is sufficiently resolved.

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: Integration speed on PRs

#3 Post by adrian_broher »

> What is your expectation for responses to and merging of pull requests?

In general: less manual intervention, more automation. Human intervention should only happen where the machine is too stupid to give proper feedback.

However with the current state of the project doesn't cover my general notion of what I would consider well testable.

> I have limited hours during a day for freeorion stuff, and I don't want to drop everything else whenever there is a reviewable pull request ready,

I understand that and by no means I want to order around you or anybody else, but I stil would like to know how to reduce the turnover of PRs in general.

So I was wondering if the workflow of providing PRs could be improved somehow (smaller, single topic, atomic; but I think I cover this already pretty well), or how the CI could be used to automate some more tasks. That's why I am wondering how your PR test workflow looks like and what are your acceptance critirias for integrating/rejecting a PR.

> If the github filtering is showing me what I want, you currently have 6 open pull requests for freeorion

I have around 15 local branches at hand in different completion states and interdependencies (not including the published ones) and I'm already trying to hold the majority of them back for some time.
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13586
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Integration speed on PRs

#4 Post by Geoff the Medio »

adrian_broher wrote: Tue Apr 07, 2020 11:10 pmSo I was wondering if the workflow of providing PRs could be improved somehow (smaller, single topic, atomic; but I think I cover this already pretty well), or how the CI could be used to automate some more tasks. That's why I am wondering how your PR test workflow looks like and what are your acceptance critirias for integrating/rejecting a PR.
Workflow is very context dependent and flexible. Scale of the changes, whether the changes seem potentially controversial or based around ongoing design discussions, and whether the author is new or well-established and trusted are considered. For you specifically, if it's a small change you're fairly certain won't cause any issues, you could just merge it yourself.

Assuming it's a fairly big and/or technically complicated code change, and the CI builds are OK, and especially if it's intended to be a gameplay or interface change, then I might check out the changes to build and test run locally, or look over the diffs in detail on github and leave comments. Other than setting up, and then maintaining, automated running of test games with hooks to check the results, I'm not sure how to automate this... Reasoning about whether a code refactoring change makes sense in the context of FreeOrion-specific use or gameplay considerations, or whether a GUI tweak or layout change is working well, are hard to write unit tests for, I suspect.

Ophiuchus
Programmer
Posts: 3427
Joined: Tue Sep 30, 2014 10:01 am
Location: Wall IV

Re: Integration speed on PRs

#5 Post by Ophiuchus »

One classical way to do quality assurance that would be a test suite which on one hand can show if regression happens (refactoring) and on the other hand if the test suite changes it shows that the intentions change. I would love to have a test suite for the combat system and build queue processing. It would help me speed up my development. But it is a huge investment I think and I do not have the cpp background to add a well-suited test framework. Also the code is not really in a good shape for testing right now (too many quasi-globals). So I do not think automation is anywhere near the horizon. @adrian_brother: But in your case geoff already stated that he would use trust in your knowledge instead. Much cheaper.

But if the project had more expert knowledge to draw on, it would make maybe sense to involve more people in code review in order to cover the technical changes. That could free up e.g. geoff to focus more on steering/making decisions/implementing influence - (@geoffthemedio if you want that). But actually I do not know who has the knowledge and resources to do that?

p.s.: local changes are not visible to anybody but yourself so I am doing PRs more often than I did.
Getting feedback is still really hard though. Often I get the necessary feedback only after merging into master. The slow game server now tested a medium game change (changes to how fighter work mostly) before merging, that was nice and a good way of getting feedback. Still that had ages of forum discussion, multiple polls and multiple PRs before. So certainly not an effective workflow. I hope to get some changes into the tenth slow game and improve iteration speed that way.
Any code or patches in anything posted here is released under the CC and GPL licences in use for the FO project.

Look, ma... four combat bouts!

User avatar
Oberlus
Cosmic Dragon
Posts: 5704
Joined: Mon Apr 10, 2017 4:25 pm

Re: Integration speed on PRs

#6 Post by Oberlus »

Maybe it would make sense to have an Unstable (or something) branch:
  • Untrusted/Unreviewed PRs (that Geoff and other members have not had time to properly review) can be merged there and tested by others for bugs without messing master.
  • Contributors and playtesters with little git knowledge can pull that branch easily without having to worry about merging, rebasing, conflicts, etc. because someone with more knowledge (members) already did that routinely.
  • Multiplayer games could use that branch.
  • Whenever a merged PR screwed things up in Unstable branch, contributors, playtesters, etc. could still use master branch without having to hunt for the PR that caused the issue.
  • That branch could be distributed along Vezzra's Weekly Test Builds, Ophiuchus' snap and o01eg's Ubuntu ppa (if they have the time and motivation to build those).

Ophiuchus
Programmer
Posts: 3427
Joined: Tue Sep 30, 2014 10:01 am
Location: Wall IV

Re: Integration speed on PRs

#7 Post by Ophiuchus »

Oberlus wrote: Wed Apr 08, 2020 10:23 am Maybe it would make sense to have an Unstable (or something) branch:
  • ...
  • That branch could be distributed along Vezzra's Weekly Test Builds, Ophiuchus' snap and o01eg's Ubuntu ppa (if they have the time and motivation to build those).
Great idea. And yes, I am totally motivated :D
Any code or patches in anything posted here is released under the CC and GPL licences in use for the FO project.

Look, ma... four combat bouts!

User avatar
Oberlus
Cosmic Dragon
Posts: 5704
Joined: Mon Apr 10, 2017 4:25 pm

Re: Integration speed on PRs

#8 Post by Oberlus »

Extending on the topic (to which I am new), I found this in a quick search:
Graduation

As a given feature goes from experimental to stable, it also "graduates" between the corresponding branches of the software. git.git uses the following integration branches:
  • maint tracks the commits that should go into the next "maintenance release", i.e., update of the last released stable version;
  • master tracks the commits that should go into the next release;
  • next is intended as a testing branch for topics being tested for stability for master.
There is a fourth official branch that is used slightly differently:
  • pu (proposed updates) is an integration branch for things that are not quite ready for inclusion yet (see "Integration Branches" below).
Each of the four branches is usually a direct descendant of the one above it.

Conceptually, the feature enters at an unstable branch (usually next or pu), and "graduates" to master for the next release once it is considered stable enough.


Merging upwards

The "downwards graduation" discussed above cannot be done by actually merging downwards, however, since that would merge all changes on the unstable branch into the stable one. Hence the following:
Rule: Merge upwards

Always commit your fixes to the oldest supported branch that require them. Then (periodically) merge the integration branches upwards into each other.

This gives a very controlled flow of fixes. If you notice that you have applied a fix to e.g. master that is also required in maint, you will need to cherry-pick it (using git-cherry-pick[1]) downwards. This will happen a few times and is nothing to worry about unless you do it very frequently.

Post Reply