Martchus / syncthingtray

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

upgraded syncthingtray (1.3.0-3 -> 1.3.0-4) Now will not connect/work at all? #163

Closed JackDinn closed 1 year ago

JackDinn commented 1 year ago

[2022-11-06T08:56:34] Connection configuration is insufficient. [2022-11-06T08:56:52] Connection configuration is insufficient. [2022-11-06T09:08:00] Connection configuration is insufficient. [2022-11-06T09:09:39] Unable to request Syncthing log: Protocol "" is unknown

ksnip_20221106-091843 It just give a blank white window when i try to "open syncthing" , its just failing in all sorts of ways but syncthing itself is running and syncing fine.

Dont know what info to supply, sorry. System: Host: greg-inspiron5767 Kernel: 6.0.6-1-MANJARO arch: x86_64 bits: 64 Desktop: KDE Plasma v: 5.26.2 Distro: Manjaro Linux

JackDinn commented 1 year ago

O sorry, i did figure it, it had lost all my configurations including the connection setup.

Martchus commented 1 year ago

Since the update only introduced two patches for tests it is unlikely a regression in Syncthing Tray itself that caused the loss of the config. However, I'm nevertheless wondering why it happened.

embeddedt commented 1 year ago

I had the exact same issue. I just started using syncthingtray a couple days ago and then upgraded it today (syncthingtray AUR package). After upgrading my config label was set to "testconfig" and all configs had been reset.

Fortunately I had hardly changed any defaults so it took only 15 seconds to restore my previous settings, but I can see this being surprising to most users.

JackDinn commented 1 year ago

Yes, "testconfig" that was how it was labelled for me also. I had quite a bit to re-configure, but after remembering how i had it all, it was not too bad.

Martchus commented 1 year ago

I had the issue myself. I'm not sure yet why it happens. Have to investigate whether running tests on current master can somehow change the normal config.

Martchus commented 1 year ago

The tests were indeed messing the user config. When changing the mocking of Syncthing's config I overlooked that this also affects the lookup of Syncthing Trays config. Besides the connection config also the launcher settings were affected so make sure to re-configure it as needed (so it doesn't spawn a "testinstance" in the background).

It should be fixed on master and I'll update the packages soon. If you want to be on the safe side you can backup syncthingtray.ini before building it (but of course I have tested whether the fix works on my system).

sten0 commented 1 year ago

I began packaging Syncthing Tray 1.3.0 today, and discovered the following test failure in the wizard, which looks related. Is this the case, or should I file a separate bug? I hope it's related, so the fixes you already made will solve it :)

2/3 Test #2: syncthingctl_run_tests ..............   Passed    2.53 sec
test 3
    Start 3: syncthingwidgets_run_wizard_tests

3: Test command: /<<PKGBUILDDIR>>/debian/build/widgets/syncthingwidgets_wizard_tests
3: Working Directory: /<<PKGBUILDDIR>>/debian/build/widgets
3: Test timeout computed to be: 10000000
3: QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-sten'
3: ********* Start testing of WizardTests *********
3: Config: Using QtTest library 5.15.6, Qt 5.15.6 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 12.2.0), debian unknown
3: QDEBUG : WizardTests::initTestCase() "HOME dir: /tmp/syncthingwidgets_wizard_tests-yAAnZO"
3: PASS   : WizardTests::initTestCase()
3: QWARN  : WizardTests::testShowingSettings() QLayout: Attempting to add QLayout "" to QtGui::DetectionWizardPage "", which already has a layout
3: PASS   : WizardTests::testShowingSettings()
3: QWARN  : WizardTests::testConfiguringLauncher() QLayout: Attempting to add QLayout "" to QtGui::DetectionWizardPage "", which already has a layout
3: QWARN  : WizardTests::testConfiguringLauncher() QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-sten'
3: QWARN  : WizardTests::testConfiguringLauncher() This plugin does not support propagateSizeHints()
3: QWARN  : WizardTests::testConfiguringLauncher() QFSFileEngine::open: No file name specified
3: Info: Launched process, PID: 574983
3: QWARN  : WizardTests::testConfiguringLauncher() QFSFileEngine::open: No file name specified
3: Info: Launched process, PID: 574990
3: QWARN  : WizardTests::testConfiguringLauncher() QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-sten'
3: QDEBUG : WizardTests::testConfiguringLauncher() waiting for Syncthing to write config file
3: FAIL!  : WizardTests::testConfiguringLauncher() Compared values are not the same
3:    Actual   (settings.connection.primary.syncthingUrl): "http://127.0.0.1:36751"
3:    Expected (QStringLiteral("http://127.0.0.1:8384")) : "http://127.0.0.1:8384"
3:    Loc: [./widgets/tests/wizard.cpp(266)]
3: QWARN  : WizardTests::testConfiguringCurrentlyRunningSyncthing() QLayout: Attempting to add QLayout "" to QtGui::DetectionWizardPage "", which already has a layout
3: Info: Launched process, PID: 575012
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(319)]
3: QDEBUG : WizardTests::cleanupTestCase() terminating Syncthing
3: PASS   : WizardTests::cleanupTestCase()
3: Totals: 3 passed, 2 failed, 0 skipped, 0 blacklisted, 7269ms
3: ********* Finished testing of WizardTests *********
3/3 Test #3: syncthingwidgets_run_wizard_tests ...***Failed    7.50 sec
Martchus commented 1 year ago

It is most likely related. I recommend to build the latest master or at least with the set of patches I've added to https://aur.archlinux.org/packages/syncthingtray.

sten0 commented 1 year ago

Martchus @.***> writes:

It is most likely related. I recommend to build the latest master or the set of patches I've added to https://aur.archlinux.org/packages/syncthingtray.

I confirm that with a30f3d406 tests pass in a clean chroot that tests for stray writes.

I'll wait for a 1.3.1, I need to wait for the 0.3.0 -> 1 ABI bump of qtforkawesome to be approved by either/both Debian ftpmasters [and] the release team. By the way, thank you for beginning to stabilise its ABI :)

Martchus commented 1 year ago

By the way, thank you for beginning to stabilise its ABI

I still don't make an extra effort to keep the ABI stable. I just avoid bumping the soname needlessly. It would be best to consider qtforkawesome an implementation detail. It is unfortunate that GNU/Linux distributions/packaging consider all kinds of libraries "system libraries" and thus apply needlessly high standards on ABI (or API) stability. Note that you likely do not apply such standards on all dependencies of Syncthing itself. At least I'd presume that because Syncthing itself uses mostly static linking for its dependencies. (Static linking is supported by my build system as well by the way.)

I confirm that with https://github.com/Martchus/syncthingtray/commit/a30f3d40625fac45320c57b9b896490a6de70662 > tests pass in a clean chroot that tests for stray writes.

Nice, thanks for testing.

sten0 commented 1 year ago

Martchus @.***> writes:

By the way, thank you for beginning to stabilise its ABI

I still don't make an extra effort to keep the ABI stable. I just avoid bumping the soname needlessly. It would be best to consider qtforkawesome an implementation detail. It is unfortunate that GNU/Linux distributions/packaging consider all kinds of libraries "system libraries" and thus applies a needlessly high standards on ABI (or API) stability.

We discussed this previously, and I wrote that I wouldn't pressure you to stabilise ABI. You may choose to consider or ignore the following:

Debian (and derivatives) will ideally ship with KDE Plasma 5.27; however, upstream KDE release is delayed and/or if manpower is short at a critical time then it's possible we'll get stuck with 5.26.

The release will happen in 2023, most users will upgrade in 2025, but some will hold out until 2026. Most of these users (60% according to current opt-in metrics) will be using the plasmoid on Plasma Desktop 5.26 or 5.27.

The reason these "needlessly high standards" exist is to better achieve the following: 1) To make users happy with high quality software.
2) For you to be happy with the state of a specific release of your software that people will use for 2-3 years on Plasma 5.26 or 5.27.

If ABI bumps are needed for cpp-utilities, qtutilities, or qtforkawesome, please make them by the 12th of December. Yes, I know 5.27 probably won't have been released yet, and that I might have to cherry pick a fix later...there will be grace for Plasma 5.27-specific targeted fixes! 5.27 is LTS, by the way.

I confirm that with https://github.com/Martchus/syncthingtray/commit/a30f3d40625fac45320c57b9b896490a6de70662 > tests pass in a clean chroot that tests for stray writes.

Nice, thanks for testing.

You're welcome :) The new qtforkawesome package was accepted today (surprisingly quickly), and I've uploaded 71675c7b1 of Syncthingtray to experimental for some early CI coverage.

Martchus commented 1 year ago

Debian (and derivatives) will ideally ship with KDE Plasma 5.27; however, upstream KDE release is delayed and/or if manpower is short at a critical time then it's possible we'll get stuck with 5.26.

The release will happen in 2023, most users will upgrade in 2025, but some will hold out until 2026. Most of these users (60% according to current opt-in metrics) will be using the plasmoid on Plasma Desktop 5.26 or 5.27.

Not sure how all of that is related to me changing the ABI of a library, though.

The reason these "needlessly high standards" exist is to better achieve the following: 1) To make users happy with high quality software. 2) For you to be happy with the state of a specific release of your software that people will use for 2-3 years on Plasma 5.26 or 5.27.

1) The quality of the software doesn't depend on whether an ABI change is happening more often or not. In fact, less hacks to preserve the ABI might even result in a higher quality. And in general, the user shouldn't be affected because e.g. one had to add a struct member and therefore the ABI was changing. It is a change like any other change and not generally more "dangerous" than non ABI-breaking changes. My point was that treating libraries that are merely an implementation details of a certain application (here of Syncthing Tray) as "platform libraries" used by the rest of the system means applying a "needlessly high standard". There's no gain from it for the quality of the software perceived by the user.

2) I guess I wouldn't be happy using them software that is so old anyways. If one of those users reports an issue I will tell them to upgrade and file an issue only against the latest version. So they're basically out of upstream support. I also don't see how that point relates to ABI changes.

If ABI bumps are needed for cpp-utilities, qtutilities, or qtforkawesome, please make them by the 12th of December. Yes, I know 5.27 probably won't have been released yet, and that I might have to cherry pick a fix later...there will be grace for Plasma 5.27-specific targeted fixes! 5.27 is LTS, by the way.

I make them as needed (independent of any external schedules which might conflict anyways) but as less as possible.

Martchus commented 1 year ago

This problem should be resolved now.