Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

syncthing: update to v1.23.4 and include next-gen-gui #5523

Merged
merged 18 commits into from
Apr 26, 2023

Conversation

hgy59
Copy link
Contributor

@hgy59 hgy59 commented Dec 16, 2022

Description

  • regarding Syncthing: accept: function not implemented #5289 we have to wait for GO v1.19.5 to include fix for ARMv5
  • update syncthing to v1.22.2
  • include next-get-gui and provide it under /tech-ui
  • fix parameter of ca_reloader.sh
  • fix link in wizard files

Fixes #5289

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix
  • New Package
  • Package update
  • Includes small framework changes
  • This change requires a documentation update (e.g. Wiki)

@hgy59
Copy link
Contributor Author

hgy59 commented Dec 16, 2022

@acolomb I plan to add configuration of a shared folder to the installation wizard. This folder will be used as the default sync folder.

Currently the package var folder is used as HOME and therefore the default sync folder (for sc-syncthing service user). As reported in #3320 this folder is not accessible by DSM users. A shared folder will be supported by DSM to configure dedicated user permissions.

I already updated syncthing in this PR to not create the default folder at first service start.

Any hint is appreciated, as I am not a syncthing user.

@acolomb
Copy link
Contributor

acolomb commented Dec 16, 2022

Well, Syncthing doesn't have or need the concept of one main folder, as it can be pointed at any accessible path in the file system. The default folder is basically just to help get people jump-started on the Desktop. It doesn't really make sense on a NAS I think, where the purpose is usually to synchronize files which are also used by other applications or sharing protocols.

Thus the most sensible solution would be to just disable creating the default folder, via --no-default-folder when first calling syncthing generate or syncthing serve in service-setup.sh.

If you want to add a "share" through the wizard, where would that be stored and how could other apps access it? I don't see how creating an empty default folder under a new share really helps with using Syncthing on the NAS. The default folder can be removed from Syncthing anytime, but the share will stay around, for what purpose?

cross/syncthing/Makefile Outdated Show resolved Hide resolved
spk/syncthing/Makefile Outdated Show resolved Hide resolved
spk/syncthing/src/ca_reloader.sh Outdated Show resolved Hide resolved
spk/syncthing/src/options.conf Outdated Show resolved Hide resolved
cross/syncthing/PLIST Outdated Show resolved Hide resolved
@hgy59
Copy link
Contributor Author

hgy59 commented Dec 17, 2022

@acolomb I am not happy with the creation of a shared folder.
What about my solution to define a custom HOME folder in options.conf?

Copy link
Contributor

@acolomb acolomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how the HOME variable can be overridden now, but I seriously doubt anyone will use it. The template folder path can already be adjusted directly in the Web GUI, and therefore $HOME is only relevant for the meaning of ~.

I haven't tested any of this yet, just looked at the code.

spk/syncthing/src/ca_reloader.sh Show resolved Hide resolved
spk/syncthing/src/service-setup.sh Outdated Show resolved Hide resolved
@acolomb
Copy link
Contributor

acolomb commented Jan 11, 2023

@hgy59 This should be updated to the latest Syncthing v1.23.0 (or whatever is the latest stable) release before merging.

@acolomb
Copy link
Contributor

acolomb commented Jan 11, 2023

@hgy59 I've now successfully built the package for aarch64, including the next-gen-gui, with Go 1.19.5. It took about four minutes mainly because of all the NPM stuff going on. Frankly I don't even know what even needs "building" there, the usual GUI just works from the Git repo state.

So I repeat my request to please leave this --with-next-gen-gui part out of the build and drop the NPM dependency.

If anyone wants to use the "tech-ui", it's as simple as unzipping its build to the theme folder (we can keep the introduced gui/ subdirectory), but the build doesn't need to happen during SPK construction IMHO. This leaves room for other drop-in themes or modified GUIs. Actually someone is currently working on modernizing the old Web GUI, so we'll see whether that ends up needing a build step, but right now please skip it for the tech-ui.

hgy59 and others added 13 commits February 12, 2023 14:35
- update syncthing to v1.22.2
- include next-get-gui and provide it under /tech-ui
- fix parameter of ca_reloader.sh
- fix link in wizard files
- avoid the creation of a default folder on first startup
Co-authored-by: André Colomb <github.com@andre.colomb.de>
Co-authored-by: André Colomb <github.com@andre.colomb.de>
- avoid the use of '--home' argument as not related to HOME env. variable
- enable to define custom HOME folder in options.conf
- add customization information to wizard
- fix install_uifile_fre (add wizard_username and wizard_password)
- add information for options.conf.new to upgrade wizard
Co-authored-by: André Colomb <github.com@andre.colomb.de>
- omit to compile synchthing --with-next-gen-gui
@hgy59 hgy59 changed the title syncthing: update to v1.22.2 and include next-gen-gui syncthing: update to v1.23.1 and include next-gen-gui Feb 12, 2023
@hgy59
Copy link
Contributor Author

hgy59 commented Feb 12, 2023

@acolomb this should be ready to merge now

@acolomb
Copy link
Contributor

acolomb commented Feb 12, 2023

Let me take a look tonight or tomorrow probably.

@hgy59
Copy link
Contributor Author

hgy59 commented Feb 12, 2023

Let me take a look tonight or tomorrow probably.

don't hurry, I will be offline for about a week. (BTW i removed the nodejs dependency by adding prebuilt tech-ui).

@hgy59
Copy link
Contributor Author

hgy59 commented Feb 25, 2023

@acolomb just a friendly reminder to have a look at this PR.

@acolomb
Copy link
Contributor

acolomb commented Feb 27, 2023

Thanks @hgy59 and sorry for taking so long. I've had some unforeseen things to take care of lately so didn't get a chance to look at code. Hopefully this week.

@agoeckner
Copy link

agoeckner commented Apr 5, 2023

If the "next gen UI" is a sticking point, can we please just perform an upgrade to v1.23.1 and relegate the UI to another PR?

@acolomb
Copy link
Contributor

acolomb commented Apr 5, 2023

I just didn't find the time to review again, but it's not forgotten and I will try to get it done very soon.

Before we merge, v1.23.3 just came out, so better include that latest version before merging. However a bug (syncthing/syncthing#8851) was just found which may lead to another point release in the next few days, will keep you posted on that.

Note that you should be on Syncthing v1.23.3 already if you haven't disabled the internal upgrade mechanism. The packaged binary keeps itself up to date regardless of the SPK package version.

@agoeckner
Copy link

Note that you should be on Syncthing v1.23.3 already if you haven't disabled the internal upgrade mechanism. The packaged binary keeps itself up to date regardless of the SPK package version.

Thanks! I forgot that I had disabled the updater. All is working well now with v1.23.4 on my DS212j.

Copy link
Contributor

@acolomb acolomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except for needing an upgrade to v1.23.4 now. Suggestions included to carry that out. Otherwise just a simple needless whitespace change.

cross/syncthing/Makefile Outdated Show resolved Hide resolved
spk/syncthing/Makefile Outdated Show resolved Hide resolved
cross/syncthing/Makefile Outdated Show resolved Hide resolved
cross/syncthing/digests Outdated Show resolved Hide resolved
spk/syncthing/Makefile Outdated Show resolved Hide resolved
hgy59 and others added 5 commits April 26, 2023 22:58
Co-authored-by: André Colomb <github.com@andre.colomb.de>
Co-authored-by: André Colomb <github.com@andre.colomb.de>
Co-authored-by: André Colomb <github.com@andre.colomb.de>
Co-authored-by: André Colomb <github.com@andre.colomb.de>
- avoid running ca_reloader.sh on DSM 7+
@hgy59 hgy59 changed the title syncthing: update to v1.23.1 and include next-gen-gui syncthing: update to v1.23.4 and include next-gen-gui Apr 26, 2023
@hgy59 hgy59 merged commit 19ec801 into SynoCommunity:master Apr 26, 2023
@hgy59 hgy59 deleted the update_syncthing branch April 26, 2023 21:28
@hgy59 hgy59 added the status/published Published and activated (may take up to 48h until visible in DSM package manager) label Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/published Published and activated (may take up to 48h until visible in DSM package manager)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Syncthing: accept: function not implemented
3 participants