*** Bug 1164378 has been marked as a duplicate of this bug. ***

Hey Jan - do you mind helping a first time go packager get off the ground?

1) Missing:
# e.g. el6 has ppc64 arch without gcc-go, so EA tag is required
ExclusiveArch:  %{?go_arches:%{go_arches}}%{!?go_arches:%{ix86} x86_64 aarch64 %{arm}}
# If go_compiler is not set to 1, there is no virtual provide. Use golang instead.
BuildRequires:  %{?go_compiler:compiler(go-compiler)}%{!?go_compiler:golang}

It is not required, it just helps to build your spec file on various architectures. The go_arches is not defined on el6 and epel7 so the build will fail there

2) Missing %commit macro. Though it is not needed to build the rpms, it is used by gofed tooling to associate your spec file with upstream repository (e.g. to run periodic checks, to bump the spec file, ...)
%global commit 204f125ab33eb5f4595343940db343ffb01108ee

3) Missing BuildRequires in the devel subpackage. Without the BuildRequires you can not run tests. Though the BR are part of the main package, they are needed in the devel subpackage as well.

4) Missing unit-test-devel subpackage. Even if the tests are not run, it is always good to provide the test. So some tooling can run CI tests over all go packages one day and collect important data for upstream feedback.

1) I replaced EA tags with the appropriate ones.

2) I put in %commit and %shortcommit that match the release tag.

3 + 4) I'll adapt the *syncthing.spec file contents generated by gofed and include the missing BRs and Reqs.


There's two more question from my side:

i) gofed generates quite a long list of BR's and Requires, but all of those are bundled in the upstream tarball (and not all of them are in fedora yet). Should I strip the "vendored" / bundled libraries before %build? (I know fedora doesn't like bundled stuff ...) If so, I'll probably have to open review requests for the packages missing from fedora, and BR: those packages.

ii) I think I should include the following Provides to match the usual naming scheme for golang packages. Is that a good idea?

- syncthing provides golang-github-syncthing-syncthing,
- syncthing-devel provides
    - golang-github-syncthing-syncthing-devel and
    - golang(github.com/syncthing/syncthing)
- syncthing-unit-test-devel provides 
    - golang-github-syncthing-syncthing-unit-test-devel

> i) gofed generates quite a long list of BR's and Requires, but all of those are
> bundled in the upstream tarball (and not all of them are in fedora yet). Should
> I strip the "vendored" / bundled libraries before %build? (I know fedora
> doesn't like bundled stuff ...) If so, I'll probably have to open review
> requests for the packages missing from fedora, and BR: those packages.

Whenever you can de-bundle please do so. You should use a dependency in vendor directory only when there is no other option. E.g.
- dependency in the distribution is not backward compatible and there is no reasonable way to patch your project
- dependency is changing very rapidly and you have no assurance of API stability (e.g. kubernetes, docker)

> ii) I think I should include the following Provides to match the usual naming
> scheme for golang packages. Is that a good idea?
>
>- syncthing provides golang-github-syncthing-syncthing,



>- syncthing-devel provides
>    - golang-github-syncthing-syncthing-devel and

Name of the devel subpackage is not important.
Every go package should/must [Build]Require virtual provides (golang(...))
So no need to introduce additional provide

>    - golang(github.com/syncthing/syncthing)

List of provided packages in devel package is generated by gofed by default.
Optionally, you can run ``gofed inspect -p --spec`` inside the tarball to get a list of all provided packages in spec format.

>- syncthing-unit-test-devel provides
>    - golang-github-syncthing-syncthing-unit-test-devel

The unit-test-devel package is never meant to be [Build]Required so no need for it.

Good, that's what I suspected to be the case. I'll submit review requests for the dependencies and mark them as blockers for this review.

I will use the -devel and -unit-test-devel subpackages (including BRs and Reqs) generated by gofed, as that seems to work really well.

Thank you for your help!

I have updated the .spec file to incorporate all suggestions / improvements. It is available at the same location as before.

The SRPM doesn't build yet, since the syncthing build script seems to be a bit broken (I reported this upstream at [1]) - it executes "go install" (for already installed libraries!) even though I run it in "build only" mode (so there seem to be 2 separate problems) ...

With the same .spec file (except setting %with_devel to 0 and %with_bundled to 1) I ran a local build for EPEL7 and that works fine.


[1]: https://github.com/syncthing/syncthing/issues/4016

I updated the .spec and built a new .src.rpm package for the recent 0.14.24 release:

Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.24-1.fc25.src.rpm


The only issue blocking the build is the too-old version of the gogoprotobuf go library (linked as blocker bug above).

Also, I'm not sure if the %systemd macros work for the units as they should, since one of them takes an argument(a username - and I don't know if @systemd_* macros work for something like "syncthing@.service").

Since some of the build dependencies for the syncthing server tools require further investigation (build failures for some architectures) - and ~10 more package reviews, if/once everything works out - I decided *not* to build the server tools if not using the bundled sources.

At the moment, fedora builds use the system golang libraries, whereas EPEL builds use the bundled / vendored sources (and the server tools are built too). With this approach, I got syncthing to build on both fedora rawhide (all arches) and EPEL 7 (on x86_64 and aarch64; only those have golang).

koji scratch build for rawhide: https://koji.fedoraproject.org/koji/taskinfo?taskID=18307514

koji scratch build for EPEL7:   https://koji.fedoraproject.org/koji/taskinfo?taskID=18307609

The .spec and .src.rpm file links from Comment 8 now point to the working version. I also needed to do some cleanup of empty files and stuff after the tests to make rpmlint happy - I suspect this might break some tests in the installed -unit-test-devel package, though.

I updated the .spec file so the server tools are now built unconditionally, since I have now submitted all the package review requests for the still missing golang packages. So strelaysrv, strelaypoolsrv and stdiscosrv will now be available on fedora too.

I've updated the packaging for the latest stable release of syncthing (v0.14.27). With all dependencies being present (such as in the COPR repository at [1], which I set up for testing this), it builds successfully.

Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.27-1.fc26.src.rpm


[1]: https://copr.fedorainfracloud.org/coprs/decathorpe/golang-staging/monitor/

I've updated the .spec and SRPM files for the newest 0.14.28 upstream release.

Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.28-1.fc26.src.rpm

Updated .spec and SRPM files for the newest 0.14.29 upstream release:

Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.29-1.fc26.src.rpm

Updated .spec and SRPM files for the newest 0.14.30 upstream release:

Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.30-1.fc26.src.rpm

Updated .spec and SRPM files for the newest 0.14.31 upstream release:

Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.31-1.fc26.src.rpm

Additionally, I have included a systemd preset file to *not* enable the syncthing user service for all users at installation, matching the default system preset present on fedora (which is missing for user services).

successful COPR build for fedora 24, 25, 26, rawhide, with all missing dependencies present:
https://copr.fedorainfracloud.org/coprs/decathorpe/golang-staging/build/571293/

Updated .spec and SRPM files for the newest 0.14.32 upstream release:

Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.32-1.fc26.src.rpm

successful COPR build for fedora 24, 25, 26, rawhide, with all missing dependencies present: https://copr.fedorainfracloud.org/coprs/decathorpe/golang-staging/build/577836/

Updated .spec and SRPM files (with a rebased patch for the build script) for the newest 0.14.33 upstream release:

Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.33-1.fc26.src.rpm

successful COPR build for f24, f25, f26, rawhide, with all missing dependencies  present: https://copr.fedorainfracloud.org/coprs/decathorpe/golang-staging/build/583592/


Comments on rpmlint error output of SRPM and RPMs:

- 3 x E: hardcoded-library-path in */usr/lib/systemd/user-preset

I can't fix this, because systemd's RPM macros don't provide a %{_userpresetdir} analog to %{_presetdir}.

- E: useless-provides debuginfo(build-id)
- E: debuginfo-without-sources

I think this is intentional (.go sources can't even be utilized for debugging go binaries). In any case, RPM is generating the -debuginfo package itself ...

I've adapted the patch to the upstream build script so all fedora-specific build flags and LDFLAGS are passed correctly, which fixes the previously broken -debuginfo and -debugsources subpackages.

Additionally, since all dependencies are in the rawhide buildroot now (repository/mirrors may take some time yet), I can provide a successful koji scratch build (ppc64 fails for obvious reasons):
https://koji.fedoraproject.org/koji/taskinfo?taskID=20941996


Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.33-2.fc26.src.rpm

My last patch to the build scripts had a small bit missing, which resulted in some variables not being set during build. It's fixed in this version:

Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.33-3.fc26.src.rpm

koji scratch build for rawhide: https://koji.fedoraproject.org/koji/taskinfo?taskID=21074310

Updated .spec and SRPM files for the newest 0.14.34 upstream release:

Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.34-1.fc26.src.rpm

koji scratch build for rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=21106043

koji scratch build for EPEL7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=21106229

COPR build for f25, f26:
https://copr.fedorainfracloud.org/coprs/decathorpe/golang-staging/build/587953/


There's just one question from my side:
Should I include Provides: bundled(golang(XXX)) for the RHEL7 build, where the .spec is currently using bundled dependencies?

Updated .spec and SRPM files for the newest hotfix 0.14.35 upstream release:

Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.35-1.fc26.src.rpm


koji scratch build for rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=21108672

koji scratch build for EPEL7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=21108666

COPR build for f25, f26:
https://copr.fedorainfracloud.org/coprs/decathorpe/golang-staging/build/588022/


My question from Comment #20 is still open.

Hi Fabio,

Sorry for the long wait here!

I am working on this review ATM. Yes, if you are shipping a bundled version for RHEL, it would be nice to provide de bundled package. Is there a reason for bundling it though?

Well ... last time I checked, golang (expecially golang(XXX) dependencies on other golang-XXX-devel packages) was a mess on RHEL/EPEL, and that's why I turned on bundled dependencies for RHEL builds.

However, after just reading more about EPEL7 packaging and the discussions around bundling, I've decided against building this package for EPEL7 (for now). So you can continue this review as fedora-only. Thanks :)

Thanks for the hard work packaging this, it is not a simple package at all :)

(In reply to Fabio Valentini from comment #15)
> Additionally, I have included a systemd preset file to *not* enable the
> syncthing user service for all users at installation, matching the default
> system preset present on fedora (which is missing for user services).

Have you thought about pinging systemd packagers about this?

(In reply to Fabio Valentini from comment #17)
> - 3 x E: hardcoded-library-path in */usr/lib/systemd/user-preset
>
> I can't fix this, because systemd's RPM macros don't provide a
> %{_userpresetdir} analog to %{_presetdir}.

Have you thought about pinging systemd upstream about this as well?

- RPM lint throws the following errors:
  syncthing-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
  syncthing.src:337: E: hardcoded-library-path in %{buildroot}/usr/lib/systemd/user-preset
  syncthing.src:338: E: hardcoded-library-path in %{buildroot}/usr/lib/systemd/user-preset/90-syncthing.preset
  syncthing.src:503: E: hardcoded-library-path in /usr/lib/systemd/user-preset/90-syncthing.preset

Which are all explained in the comments above. You could probably use %{_prefix} for the path to silence rpmlint, but I believe this macro could be included in systemd macros in the future.

- /usr/lib/systemd/user-preset dir has no owner, although it belongs to the fedora-release package (I have no idea on why fedora-review triggered this)

The package looks good! Before approving, I need to check if shipping the gui resources in the binary file is ok:

The binary file has the gui/default/vendor projects embedded in it, which contains

angular
 - angular 1.2.9
 - angular-translate 2.9.0.1
 - angular-translate-loader-static-files 2.11.0
 - angular-dirPagination 759009c
bootstrap
font-awesome
jquery

You will probably need to provide the bundled projects though. I checked copr-frontend as an example for that, and it does provide

bundled(bootstrap) = 3.3.4
bundled(bootstrap-combobox) = 1.1.6
bundled(bootstrap-select) = 1.5.4
bundled(bootstrap-treeview) = 1.0.1
bundled(font-awesome) = 1.0.1
bundled(jquery) = 1.11.3
bundled(jquery-ui) = 1.11.4

> Thanks for the hard work packaging this, it is not a simple package at all :)

Yeah, it's not so easy, but hugo isn't simple, either. At least you don't have to patch build scripts. syncthing upstream was a bit "uncooperative" about fixing some things ...

> Have you thought about pinging systemd packagers about this?

I have, a month ago. Without any reaction so far.
https://bugzilla.redhat.com/show_bug.cgi?id=1468501

> Have you thought about pinging systemd upstream about this as well?

I have now, thanks for bugging me.
https://bugzilla.redhat.com/show_bug.cgi?id=1479580

> The binary file has the gui/default/vendor projects embedded in it, which contains (...)

I didn't even notice that it bundles this stuff. If you decide that using the bundled stuff is fine, I'll add those lines (versions according to source code in the tarball):

Provides: bundled(angular) = 1.2.9
Provides: bundled(angular-dirPagination) = 759009c
Provides: bundled(angular-translate) = 2.9.0.1
Provides: bundled(angular-translate-loader-static-files) = 2.11.0
Provides: bundled(bootstrap) = 3.3.5
Provides: bundled(font-awesome) = 4.5.0
Provides: bundled(jquery) = 2.2.2

The only question is whether to add this to the main package or the -devel subpackage (or both).

> syncthing upstream was a bit "uncooperative"
> about fixing some things ...

oh :(

> > Have you thought about pinging systemd packagers about this?
> 
> I have, a month ago. Without any reaction so far.
> https://bugzilla.redhat.com/show_bug.cgi?id=1468501
> 
> > Have you thought about pinging systemd upstream about this as well?
> 
> I have now, thanks for bugging me.
> https://bugzilla.redhat.com/show_bug.cgi?id=1479580

That's good enough, let's proceed with approving this review


> The only question is whether to add this to the main package or the -devel
> subpackage (or both).

They are nly shipped in the source tarball and in the binary package, so adding it in the Provides in the main package shall be enough.

In this case, I believe you would need to add the licenses of the bundled libraries in the License tag of the package. If you want, you can even set the different License tag only for the package with the bundled code.

Note that font-awesome is also bundling the fonts (which most of the ruby gems also do, for a different reason) and those have the Open Font License.

So the license tag should read

MPLv2.0 and MIT and OFL

(In reply to Fabio Valentini from comment #17)
> - 3 x E: hardcoded-library-path in */usr/lib/systemd/user-preset
>
> I can't fix this, because systemd's RPM macros don't provide a
> %{_userpresetdir} analog to %{_presetdir}.

Why you at least don't use:

%{_usr}/lib/systemd/user-preset

or may be better:

%{_presetdir}/../user-preset

And why you can't use system-preset? If rpm macros are set to system-preset, then I think this should be used. I am not an systemd expert and didn't find anything about system-preset vs user-preset, but I think user-preset is for users and system-preset for packages installed by system, which includes rpm packages.

> They are nly shipped in the source tarball and in the binary package, so adding it in the Provides in the main package shall be enough.

Done.

> So the license tag should read "MPLv2.0 and MIT and OFL"

Done.

> Why you at least don't use:
> %{_usr}/lib/systemd/user-preset

I've used "%{_prefix}/lib/systemd/user-preset" for now. The missing macro has already been added in systemd git master [1], so this will be fixed at some point in the future.

> And why you can't use system-preset?

I can't, because "system-preset" is for system-wide services, and I need a preset file for user-session services, which are stored in "user-preset".


Spec URL: https://decathorpe.fedorapeople.org/packages/syncthing.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/syncthing-0.14.35-2.fc26.src.rpm


I've also added "Provides: bundled(X)" for the (currently unused) %with_bundled case, just for completeness.

[1]: https://github.com/systemd/systemd/pull/6571

- License is OK
- Requires and Provides are OK
- The package provides the bundled packages used for the gui, as required
- RPMlint still raises an error about hardcoded path, which shall be fixed in future releases of the package, since the package already contacted systemd upstream about the missing macro.

The package looks good to me.


Approved.

Thank you very much for the review! :)

(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/syncthing

(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/syncthing

syncthing-0.14.36-2.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-af8b6b4204

syncthing-0.14.36-2.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-407a2d92db

syncthing-0.14.36-2.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-af8b6b4204

syncthing-0.14.36-2.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

syncthing-0.14.36-2.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.