Project

General

Profile

Merge moratorium and fixing a recent bug

Added by Delta Mike Charlie over 1 year ago

One of my recent changes has resulted in a bug:

https://tvheadend.org/issues/6293
https://tvheadend.org/boards/5/topics/50218

I have already completed troubleshooting and have issued a workaround.

This is the same change that causes issues on some systems with the embedded variable declaration within a FOR statement.

https://github.com/tvheadend/tvheadend/pull/1544

Flole Systems - You recently said that you would not merge any new changes until Oliver did his thing. I can see two options: Would you like me to?:

  1. Continue to wait for Oliver before proceeding with both fixes.
  2. Fix both problems and submit a PR ASAP.

I take full responsibility for this error as it resulted from insufficient system knowledge and testing on my part. I plan to look into the Wiki to see if I can make a note somewhere for future developers so that they can hopefully avoid a repeat of the situation.

The TVH settings subsystem seems to only load settings that are already in the settings file. When my modified version ran on a machine other than mine, it saw no corresponding setting so the internal setting update function was not triggered for that setting and the translation array was not initiated to a 1:1 state.

I know that this project is under-resourced, and I don’t have a solution for that. However, a second set of eyes would have probably caught this. It only would have taken 1 other person with a DVB system to install and run the release to spot the error and stop it. It compiled OK for the most part, but in the end the error was in the logic and I was let down by my insufficient knowledge of the nuances of the TVH internals and insufficient testing.

Luckily, 'only' 2 end users seem to have been impacted and they managed to implement the workaround fairly quickly. Although the impact was relatively small, I am still disappointed that it happened at all.


Replies (8)

RE: Merge moratorium and fixing a recent bug - Added by Flole Systems over 1 year ago

I have one PR that I want to get in before everything else to fix all the CI builds. Once that is done you can create the pull request and it will automatically checked against 45 different arch/distro variants.

RE: Merge moratorium and fixing a recent bug - Added by Delta Mike Charlie over 1 year ago

Flole Systems wrote:

I have one PR that I want to get in before everything else to fix all the CI builds. Once that is done you can create the pull request and it will automatically checked against 45 different arch/distro variants.

OK thanks. I worked on the bug fix today and tested, tested, tested and then did a bit more testing and it should be good now.

Once you let me know that you have finished, I will do a local sync and rebase and then submit the PR.

RE: Merge moratorium and fixing a recent bug - Added by Flole Systems over 1 year ago

I am done now, feel free to create the PR now.

RE: Merge moratorium and fixing a recent bug - Added by saen acro about 1 year ago

What about old PR's?
They stay with old broken status.

RE: Merge moratorium and fixing a recent bug - Added by Flole Systems about 1 year ago

Old PRs should be rebased on latest master.

RE: Merge moratorium and fixing a recent bug - Added by Delta Mike Charlie about 1 year ago

Flole Systems wrote:

I am done now, feel free to create the PR now.

Thanks Flole Systems

I have submitted my PR and it started to build on 40+ platforms. I saw that 4 of those builds had failed early only to find a second for loop with the variable declaration within the loop. PR fixed and resubmitted.

RE: Merge moratorium and fixing a recent bug - Added by Delta Mike Charlie about 1 year ago

Flole Systems wrote:

Unfortunately something else is broken aswell: https://tvheadend.org/issues/6297

I have submitted a PR to address this. Profound apologies.

    (1-8/8)