0.4.7.1 Bugfix Release: Which PRs/bugfix commits to include

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

Moderator: Committer

Post Reply
Message
Author
User avatar
Vezzra
Release Manager, Design
Posts: 4673
Joined: Wed Nov 16, 2011 12:56 pm
Location: Sol III

0.4.7.1 Bugfix Release: Which PRs/bugfix commits to include

#1 Post by Vezzra » Fri Aug 11, 2017 1:01 pm

This topic is dedicated to the discussion of PRs/bugfix commits which have been proposed for inclusion into the 0.4.7.1 bugfix release.

If you think a PR or a bugfix commit should be included into 0.4.7.1, please create a respective entry in the "Proposed" column of the 0.4.7.1 Bugfix Release project on github, and also post a request explaining why you think that PR/bugfix commit should be included here in this thread. Then the proposed inclusion of that PR/bugfix commit shall be discussed here, and a decision should be reached whether the proposal is accepted or rejected.

An accepted PR/bugfix commit will have it's entry in the 0.4.7.1 Bugfix Release project moved to the "Accepted" column. PRs will also be set to the 0.4.7.1 Bugfix Release milestone and be tagged as "cherry-pick for release". A rejected PR/bugfix commit will have it's entry in the 0.4.7.1 Bugfix Release project moved to the "Rejected" column.

Conditions a PR/bugfix commit must meet to be eligible for proposal:
  • It must not have been proposed before (that is, not be in any columns of the 0.4.7.1 Bugfix Release project).
  • It must be a pure bugfix. No PRs/commits that introduce any kind of new feature, mere tweak, etc.
  • It must be a fix for a bug in the 0.4.7 release, not something that has been introduced in master after the creation of the original 0.4.7 release branch.

User avatar
Vezzra
Release Manager, Design
Posts: 4673
Joined: Wed Nov 16, 2011 12:56 pm
Location: Sol III

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#2 Post by Vezzra » Fri Aug 11, 2017 1:23 pm

Two PRs I want to propose for inclusion (I also mentioned them in the "0.4.7.1 bugfix release?" thread, where a brief discussion already took place):

PR#1603: The issue, while not outright game breaking, is serious/annoying enough and it's fix simple enough to warrant inclusion in a bugfix release IMO. Potentially getting predictions that far off the mark can lead to misguided decisions if you're not aware of the issue.

PR#1599: I think Dilvish summed up the reasons why this should be included pretty well, so I'm just going to quote his statement in the other thread:
Dilvish wrote:The implementation in 0.4.7 has a number of clear flaws that I don't think anyone has argued against being considered flaws: (a) it implemented a significant design change without any discussion and without even being reflected in the commit message (they just referred to grooming and a dump message) as if it sort of slipped in by accident, (b) it imposes a penalty that is often significantly more severe than is even needed to achieve the design goal cited in its support, and (c) it imposes that new penalty silently, without warning to the folks who would expect things to keep working like they had.

The implementation in master, which I think for the purposes of this discussion is essentially the same as it had been for years without complaint from anyone, including yourself Geoff, is one that at least myself and Mat and Vezzra consider to be a reasonable balancing of multiple design considerations.

User avatar
MatGB
Creative Contributor
Posts: 3310
Joined: Fri Jun 28, 2013 11:45 pm

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#3 Post by MatGB » Fri Aug 11, 2017 6:00 pm

If we're doing one, then Move Phototrophic effects evaluation after all other effects by Morlic-fo · Pull Request #1615 · freeorion/freeorion and Prevent unowned monsters from attacking adequately stealthed ships by dbenage-cx · Pull Request #1582 · freeorion/freeorion would both make sense, I was never able to replicate either problem so they were real edge case problems but if you're in that edge case I can see it would be a pain.

Agree on the other two, I should've spotted the proportional production change while testing.
Mat Bowles

Any code or patches in anything posted here is released under the CC and GPL licences in use for the FO project.

Morlic
AI Contributor
Posts: 250
Joined: Tue Feb 17, 2015 11:54 am

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#4 Post by Morlic » Fri Aug 11, 2017 6:47 pm

Series of commits that fix the AI max population calculation for colonization planning:

https://github.com/freeorion/freeorion/ ... a3ced4cc7e
https://github.com/freeorion/freeorion/ ... ab6c829a57
https://github.com/freeorion/freeorion/ ... 227fa31be6
https://github.com/freeorion/freeorion/ ... acd69f7eef

Was related to e.g. a bug discussed in this thread: http://freeorion.org/forum/viewtopic.php?f=28&t=10630

The maximum note size prevented me from adding a single entry to the proposed column...
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Dilvish
AI Lead, Programmer
Posts: 4719
Joined: Sat Sep 22, 2012 6:25 pm

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#5 Post by Dilvish » Sat Aug 12, 2017 2:56 am

Although it has not yet been accepted/merged, and surely would merit some more testing that what I have done myself so far, I think that the newly submitted fix for "false" mandatory fleet stops should at least be considered for inclusion. Although the problem being dealt with there is fairly minor, it is also a fairly obvious vexation which would be nice to have cleared up.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Vezzra
Release Manager, Design
Posts: 4673
Joined: Wed Nov 16, 2011 12:56 pm
Location: Sol III

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#6 Post by Vezzra » Sun Aug 13, 2017 5:44 pm

@Mat: I've added the two PRs you suggested to the 0.4.7.1 bugfix release project.
Morlic wrote:The maximum note size prevented me from adding a single entry to the proposed column...
No problem.
Dilvish wrote:Although it has not yet been accepted/merged, and surely would merit some more testing that what I have done myself so far, I think that the newly submitted fix for "false" mandatory fleet stops should at least be considered for inclusion. Although the problem being dealt with there is fairly minor, it is also a fairly obvious vexation which would be nice to have cleared up.
As the fix is very simple, sounds reasonable. However, it needs to go in in time.

@Everyone, please take the time to at least briefly look at the PRs/bugfix commits proposed so far, and voice your objections if you have any. If possible within the next week.

User avatar
Vezzra
Release Manager, Design
Posts: 4673
Joined: Wed Nov 16, 2011 12:56 pm
Location: Sol III

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#7 Post by Vezzra » Sun Aug 20, 2017 3:47 pm

As there have been no objections against the bugfixes proposed for 0.4.7.1 so far, I went ahead and accepted all of them. 0.4.7.1. RC1 has been scheduled for coming Thursday, August 24th, provided no more bugfixes will be suggested (or those that are can be accepted in time). Otherwise RC1 (and the entire schedule for 0.4.7.1 of course) will be postponed accordingly.

So, if anyone wants to propose further bugfixes for inclusion into 0.4.7.1, please do so until Wednesday evening (UTC) at the very latest.

User avatar
Vezzra
Release Manager, Design
Posts: 4673
Joined: Wed Nov 16, 2011 12:56 pm
Location: Sol III

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#8 Post by Vezzra » Sun Aug 20, 2017 5:21 pm

When trying to cherry pick this I found out that this PR fixed a bug introduced after the 0.4.7 release, so it does not apply for the bugfix release.

Consequently has been rejected.

User avatar
MatGB
Creative Contributor
Posts: 3310
Joined: Fri Jun 28, 2013 11:45 pm

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#9 Post by MatGB » Sun Aug 20, 2017 11:35 pm

Fair enough, it was one of those edge case "never seen that myself" bugs that I knew was there, didn't know when it came in.
Mat Bowles

Any code or patches in anything posted here is released under the CC and GPL licences in use for the FO project.

User avatar
Dilvish
AI Lead, Programmer
Posts: 4719
Joined: Sat Sep 22, 2012 6:25 pm

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#10 Post by Dilvish » Sat Aug 26, 2017 5:35 pm

I am noting dbenage-cx has proposed his starlane fix. Although he only suggested it if something else prompts another RC, this sounds to me like a significant enough fix to merit doing another RC just for it (the issue may be rarely problematic, but it is very problematic on the affected platform).
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Vezzra
Release Manager, Design
Posts: 4673
Joined: Wed Nov 16, 2011 12:56 pm
Location: Sol III

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#11 Post by Vezzra » Sun Aug 27, 2017 11:01 am

I'm a bit torn on that. Yes, it's definitely a serious issue on those system where it actually turns up, but, as mentioned, it rarely turns up, and please keep in mind that I can't do a RC2 before Sunday, September 3rd, due to me being away until then. Then roughly another week to give some time to test RC2, which means declaring the release will be postponed at least a week.

So I'd like the others to vote on this proposal: RC2 even if nothing else turns up or not? Is getting in a fix for an issue that, altough serious, only rarely turns up, worth a week's delay?

User avatar
Dilvish
AI Lead, Programmer
Posts: 4719
Joined: Sat Sep 22, 2012 6:25 pm

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#12 Post by Dilvish » Sun Aug 27, 2017 8:42 pm

Vezzra wrote:Then roughly another week to give some time to test RC2, which means declaring the release will be postponed at least a week.
To me, an extra week's delay does not seem like a big cost for a more thorough bugfix release. Although it's possible that this issue is heavily hardware dependent or may have other aspects limiting the scope of the problem, we don't really know and don't really want to take extra time to figure that out. Fedora 26 is going to be supported for about another year (so would hopefully distribute this bugfix release), and Fedora 27 might easily have this same issue and is due out sometime within about 6 months, perhaps too soon for them to go with 0.4.8.

That said, if dbenage-cx has some reason to think this is just a very flukey Fedora-hardware combo problem, perhaps I am just tilting at a windmill.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Vezzra
Release Manager, Design
Posts: 4673
Joined: Wed Nov 16, 2011 12:56 pm
Location: Sol III

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#13 Post by Vezzra » Mon Aug 28, 2017 11:38 am

Sounds reasonable. Ok, unless anyone objects, I'm going to accept the proposed bugfix for 0.4.7.1 on Sunday and produce RC2.

LGM-Doyle
Programmer
Posts: 219
Joined: Mon Feb 29, 2016 8:37 pm

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#14 Post by LGM-Doyle » Mon Aug 28, 2017 3:32 pm

I agree with Dilvish the starlane fix should be included even if it delays the bugfix release.

Of all the candidates for inclusion in a bugfix relase, I think it has the strongest case. The bug makes the game difficult to play and the fix is of limited scope.

User avatar
Vezzra
Release Manager, Design
Posts: 4673
Joined: Wed Nov 16, 2011 12:56 pm
Location: Sol III

Re: 0.4.7.1 Bugfix Release: Which PRs/bugfix commits to incl

#15 Post by Vezzra » Sun Sep 03, 2017 1:15 pm

PR#1695 has been accepted and cherry picked.

Post Reply