search

Martchus / syncthingtray

Tray application and Dolphin/Plasma integration for Syncthing
https://martchus.github.io/syncthingtray/
Other
1.54k stars 43 forks source link

syncthingwidgets_run_wizard_tests on some architectures #175

Closed sten0 closed 10 months ago

sten0 commented 1 year ago

Hi! Sorry I can't remember if this test existed in 1.3.1, but 1.3.2 is blocked in Debian because this test is failing on armhf and mipsel, and 1.3.2 is marked as a regressed version. The relevant portion of the log is:

3: PASS   : WizardTests::testConfiguringLauncher()
3: Info: Launched process, PID: 541
3: QWARN  : WizardTests::testConfiguringCurrentlyRunningSyncthing() QFSFileEngine::open: No file name specified
3: QWARN  : WizardTests::testConfiguringCurrentlyRunningSyncthing() QNetworkReplyImplPrivate::error: Internal problem, this method must only be called once.
3: FAIL!  : WizardTests::testConfiguringCurrentlyRunningSyncthing() '!cfgCurrentlyRunningRadioButton->isHidden()' returned FALSE. ()
3:    Loc: [./widgets/tests/wizard.cpp(345)]
3: QDEBUG : WizardTests::cleanupTestCase() terminating Syncthing
3: QDEBUG : WizardTests::cleanupTestCase() Syncthing log during testrun:
... [snip] ...
The following tests FAILED:
      3 - syncthingwidgets_run_wizard_tests (Failed)

https://buildd.debian.org/status/fetch.php?pkg=syncthingtray&arch=mipsel&ver=1.3.2-1&stamp=1675830500&raw=0 https://buildd.debian.org/status/fetch.php?pkg=syncthingtray&arch=mipsel&ver=1.3.2-1&stamp=1673097135&raw=0 https://tests.reproducible-builds.org/debian/logs/unstable/armhf/syncthingtray_1.3.2-1.build2.log.gz

And here's a log from amd64 that passes: https://buildd.debian.org/status/fetch.php?pkg=syncthingtray&arch=amd64&ver=1.3.2-1&stamp=1673067183&raw=0

The versions of all dependencies are noted in the logs.

@Martchus, maybe you've already fixed this? Maybe 704f98e2b fixes it?

In Debian we're three days into the Soft Freeze, so this is the last call for a bugfix release. Technically the deadline was three days ago, but I want to advocate for one :) After this week, I'll only be able to make targeted fixes (cherry picks and backports). Let me know what you'd like to do.

Kind regards, Nicholas

sten0 commented 1 year ago

P.S. I'm trying to manually get a more descriptive log (and hopefully backtrace) now

Martchus commented 1 year ago

Try again with the latest version on Git. There was a race condition which should already be fixed. You've already found the relevant commit yourself so you might want to cherry-pick it.

sten0 commented 1 year ago

Try again with the latest version on Git. There was a race condition which should already be fixed.

Try what again? It's too late in the release cycle to upload a random git snapshot.

Is the master branch in a release candidate state? If not, I can cherry pick the commit noted in my initial report. If I do nothing, users get 1.3.1 for two years.

Martchus commented 1 year ago

I don't do release candidates. You can consider the master branch to be a release candidate (as experimental/incomplete developments happen on dedicated feature branches or the features are put behind the --wip CLI option). You may want to cherry-pick https://github.com/Martchus/syncthingtray/commit/704f98e2b199f260ec91b10ac7de2e7452e62018 instead of using the master branch or waiting for the next release. Note that users won't be impacted by the race condition because it only affects the test suite. What users are currently impacted with is the bug #174 but unfortunately I don't know how to reproduce it and thus cannot really work an a fix. Note that it is bad leaving users with a particular version for so long. I'll have to dismiss all issues filed against 1.3.1 once the next release is tagged because giving support for older version really exceeds the time I can spend on this project.

sten0 commented 1 year ago

Martchus @.***> writes:

I don't do release candidates. You can consider the master branch to be a release candidate. You may want to cherry-pick https://github.com/Martchus/syncthingtray/commit/704f98e2b199f260ec91b10ac7de2e7452e62018 instead of using the master branch or waiting for the next release. Note that users won't be impacted by the race condition because it only affects the test suite. What users are currently impacted with is the bug #174 but unfortunately I don't know how to reproduce it and thus cannot really work an a fix. Note that it is bad leaving users with a particular version for so long. I'll have to dismiss all issues filed against 1.3.1 once the next release is tagged because giving support for older version really exceeds the time I can spend on this project.

Unfortunately that commit appears to depend on other changes and this makes 1.3.2 fail to build.

I understand that you're primarily interested in supporting git snapshots, plus the latest release or two, and then only on rolling release distributions. The compromise I am offering is the option to import a new release during the freeze. Deadline of 23 February.

I've reuploaded 1.3.2 in good faith. In the absence of a new stable release before the 23rd, and if the race or #174 affects users, then the options available to me are 1) Make a targeted fix for the bug, or 2) Disable the wizard and make it inaccessible to users, or 3) Fall back to 1.3.1, which doesn't have the race nor issue #174.

At this point of the freeze, the goalposts won't move, and the stack is:

Qt 5.15.8 boost 1.74.0.3 KDE Plasma 5.27 (needs some time to propagate to the mirrors)

Martchus commented 1 year ago

Fall back to 1.3.1, which doesn't have the race nor issue #174.

Just for the record, this is not true. The race condition in the test suite has already existed in v1.3.1. It has existed since I have been adding those tests (when I remember correctly, even before v1.3.0). And again, this race condition was only in test code, not the code under test (so users are really not affected by this).

About https://github.com/Martchus/syncthingtray/issues/174: Since I cannot reproduce the issue myself it is impossible to tell for me since when it has been present. The user was using v1.3.2 but it is likely that this issue was introduced in an earlier version. Likely this has never worked. I will ask the user in #174 since when he can reproduce the issue.

Martchus commented 1 year ago

1) Make a targeted fix for the bug, or 2) Disable the wizard and make it inaccessible to users, or 3) Fall back to 1.3.1, which doesn't have the race nor issue #174.

I think 2) would be very easy, you just need to patch out a few lines of code. Maybe you could also show a message box instead so it will be obvious to the user that this is has been disabled downsteam. Of course 1) would be the best solution. If I can come up with a fix for #174 at some point I may be able to backport that patch for you. As mentined in my previous comment, 3) is unlikely to be helpful.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sten0 commented 1 year ago

Let's keep this one open a little while longer. Real-world testing by less adventurous users has only just started, and it will be a good reminder to reenable these tests in CI in several months (post bookworm release), and to verify if the issue is still current then.

sten0 commented 1 year ago

Martchus @.***> writes:

Reopened #175.

Thanks! 'looks like the web-interface doesn't reset a counter either.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sten0 commented 1 year ago

Hi @Martchus, would you please remove the stale label for this issue? When I find time to package the latest version of Syncthing Tray (and its deps), I'll reenable CI for the wizard. At that time, I also hope to test to see if #174 can still be triggered with manual testing--time willing. Would you please consider reopening that issue too?

stale[bot] commented 10 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sten0 commented 10 months ago

"stale[bot]" @.***> writes:

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

A couple days ago it was announced that mipsel is been dropped, and armhf (32bit hard-float ARMv7) is also mostly useful for fuzzing. @Martchus, I'll leave it to you about whether you'd prefer the continuity of keeping CI issues related to this bug at this but, or if you'd prefer for me to file a new issue when I finish testing the latest release, upload, and reenable full CI...if any issue still exists. 'hopefully none do by now! :)

Martchus commented 10 months ago

I'd say let's close this for now. If this or a similar problem happens again we can reopen this issue or just create a new issue.