Bug 1272248

Created attachment 1083553 [details]
Update license and %files section

Created attachment 1083620 [details]
update spec file

Created attachment 1083621 [details]
patch Makefile

Created attachment 1083638 [details]
update spec file

Hi Luis,

spec files looks fine. Just some nits:
- for license, %license tag can be used when redefining the tag explicitly for el6. 
- for %{gopath}/src/%{import_path} directory, it is better to include it in devel.file-list file as the root directory could contain both source files and tests file. Thus resulting in multiple ownership of the directory.
- when building heketi from bundled deps, it fails to build due to missing heketi source codes
- when building with debuginfo, bundled build is missing BUILD_ID

(In reply to Jan Chaloupka from comment #5)
> Hi Luis,
> 
> spec files looks fine. Just some nits:
> - for license, %license tag can be used when redefining the tag explicitly
> for el6. 
> - for %{gopath}/src/%{import_path} directory, it is better to include it in
> devel.file-list file as the root directory could contain both source files
> and tests file. Thus resulting in multiple ownership of the directory.
> - when building heketi from bundled deps, it fails to build due to missing
> heketi source codes
> - when building with debuginfo, bundled build is missing BUILD_ID

Hi Jan, Do you mind providing line numbers to the first two? Any suggestions on how to fix any of these issues are welcomed.  I used gofed, then based the rest of my changes on etcd.spec.

Does with_bundled mean "with bundled dependencies"?  If it does, then since Heketi uses glock it does not support that and I probably should remove that switch. 

Is with_bundled needed for centos/rhel?

Hi Luis,

all suggested changes are applied in "update spec file" patch. "patch Makefile" patches Makefile to build binaries with BUILD ID.

Jan

> Is with_bundled needed for centos/rhel?

As RHEL does not build from deps, yes. The same holds for CentOS.

Thanks for the help Jan. I have a new update available.  I did not need to change the Makefile, but thank you for the diff.  That helped create the appropriate changes to the spec file.

Here is the diff from previews request:
https://github.com/lpabon/heketi-rpm/compare/f3ea4d3c199a506651b00698709fd50475eeeac9...master

## Output from gofed

Spec URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi.spec

SRPM URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi-1.0.0-1.fc22.src.rpm

Description: Heketi provides a RESTful management interface which can be used to manage
the life cycle of GlusterFS volumes.  With Heketi, cloud services like
OpenStack Manila, Kubernetes, and OpenShift can dynamically provision
GlusterFS volumes with any of the supported durability types.  Heketi
will automatically determine the location for bricks across the cluster,
making sure to place bricks and its replicas across different failure
domains.  Heketi also supports any number of GlusterFS clusters, allowing
cloud services to provide network file storage without being limited to a
single GlusterFS cluster.

Fedora Account System Username: lpabon

$ rpmlint heketi-1.0.0-1.fc22.src.rpm heketi-1.0.0-1.fc22.x86_64.rpm heketi-devel-1.0.0-1.fc22.noarch.rpm heketi-unit-test-devel-1.0.0-1.fc22.x86_64.rpm heketi-debuginfo-1.0.0-1.fc22.x86_64.rpm
heketi.src: W: invalid-url Source3: heketi-godeps-3f4a5b1.tar.gz
heketi.x86_64: W: non-standard-uid /var/lib/heketi heketi
heketi.x86_64: W: non-standard-gid /var/lib/heketi heketi
heketi.x86_64: W: no-manual-page-for-binary heketi
heketi.x86_64: W: no-manual-page-for-binary heketi-cli
5 packages and 0 specfiles checked; 0 errors, 5 warnings.

## Output from rpmlint from CentOS 7 using bundled mode:

$ rpmlint /home/lpabon/rpmbuild/SRPMS/heketi-1.0.0-1.el7.centos.src.rpm /home/lpabon/rpmbuild/RPMS/x86_64/heketi-1.0.0-1.el7.centos.x86_64.rpm /home/lpabon/rpmbuild/RPMS/noarch/heketi-devel-1.0.0-1.el7.centos.noarch.rpm /home/lpabon/rpmbuild/RPMS/x86_64/heketi-unit-test-devel-1.0.0-1.el7.centos.x86_64.rpm

heketi.src: W: invalid-url Source3: heketi-godeps-3f4a5b1.tar.gz
heketi.x86_64: W: incoherent-version-in-changelog 1.0.0-1 ['1.0.0-1.el7.centos', '1.0.0-1.centos']
heketi.x86_64: W: unstripped-binary-or-object /usr/bin/heketi-cli
heketi.x86_64: W: unstripped-binary-or-object /usr/bin/heketi
heketi.x86_64: W: only-non-binary-in-usr-lib
heketi.x86_64: W: non-standard-uid /var/lib/heketi heketi
heketi.x86_64: W: non-standard-gid /var/lib/heketi heketi
heketi.x86_64: W: no-manual-page-for-binary heketi
heketi.x86_64: W: no-manual-page-for-binary heketi-cli
4 packages and 0 specfiles checked; 0 errors, 9 warnings.

Add) %if 0%{?with_check} && ! 0%{?with_bundled}

Nice! Applied in specfile generator, thanks.

Add) go test -v %{import_path}/apps/glusterfs

Purpose of %gotest macro is portability to secondary architectures where gcc-go is used instead of gc. On golang architectures, %gotest is defined as "go test ...". On gcc-go it is defined as "go test -compiler gcc-go ...". If you are targeting primary architectures, it is fine. For secondary, please use %gotest macro instead and define %gotest macro in %check section before the first use of %gotest as:

%if ! 0%{?gotest:1}
%global gotest go test
%endif

Otherwise the rest looks fine. Thanks Luis for your feedback on gofed. I have learnt new things from you.

Jan

Actually, one more question: what is %{name}-godeps-%{shortcommit}.tar.gz used for? What does it contain?

%{name}-godeps-%{shortcommit}.tar.gz  is a tar file containing all the dependencies, like Godeps for etcd.  The difference is that Heketi does not use Godeps, instead it uses glock so the repo does not contain the dependencies.  Instead I have provided the glock output as a tar file that is attached to the SRPM for bundled builds.  Is that ok?

Would not it be better to use _vendor or vendor directory for it? And put it all into one tarball?

One one hand creating vendor directory adds responsibility to update the directory at the same time you regenerate GLOCKFILE. On the other separating godeps from the project itself adds responsibility on maintainer of the package in Fedora not to forget to update the godeps.tar.gz file.

I would choose the former. It simplifies the spec file and it is common to provide such directory.

In both cases, if you build you package from bundled deps, you must provide a list of bundled dependencies [1]:

"All packages whose upstreams have no mechanism to build against system libraries may opt to carry bundled libraries, but if they do, they must include Provides: bundled(<libname>) = <version> in their RPM spec file."

If a file with dependencies is provided, it makes sense to replace <version> with a version which would be provided if the dependency would be packaged in Fedora. However, usually there is no version. Maybe something like %{version}-%{shortcommit} could be used instead. I have opened RFE for it [2]. What do you think about it, Luis?


[1] https://fedorahosted.org/fesco/ticket/1483#comment:17
[2] https://github.com/gofed/gofed/issues/42

If I create a _vendor directory inside the heketi-{ver}.tar.gz file, wouldn't it have to be then created by someone other than the developer since the repo will not have this direstory?  Could gofed create the godeps directory and add it inside the tar file for those programs that do not include it in their repo and instead use glock?

What do you think about the following updates:
1. add bundled(...) = <version> as you describe in [2] above
2. Keep the godeps file as separate.  When gofed can look at a repo and notice the GLOCKFILE, it can then generate a godeps.tar.gz and place it inside the {repo}.tar.gz for bundle builds.

or 

2. Setup godeps as a _vendor directory inside the heketi-{ver}.tar.gz file

What do you think?

> If I create a _vendor directory inside the heketi-{ver}.tar.gz file, wouldn't
> it have to be then created by someone other than the developer since the repo
> will not have this direstory?

With some automatic scripting it could be created in heketi's repository automatically with every update of glockfile. That was my thought.

> Could gofed create the godeps directory and add it inside the tar file for
> those programs that do not include it in their repo and instead use glock?

I could script it, actually it is a good idea. There is a lot of projects without Godeps/vendor directory suited for this use case.

I really like the first option :). Let's do it. Meantime the bundled(...) list can be generated from GLOCKFILE.

As long as the package is built from dependencies in Fedora, there is no need for bundled list. So right now the spec file conforms to the current packaging guidelines for go. Approving the spec file.

Still, please generate the bundled list under a BuildRequires of main package with:
%if 0%{?with_bundled} && 0%{?fedora}
Provides: bundled...
...
%endif

This policy is currently only for Fedora. Thanks for the discussion.

[1] https://github.com/gofed/gofed/issues/66

I made the change as you requested above.  I used the shortcommit value only because the version didn't seem to make sense in this model since some of these libraries do not version their source code.  Instead I tried to keep it simple and provide just the shortcommit.

Do you want this model or should I change it to something like: 0-%{shortcommit}?

Diff:
https://github.com/lpabon/heketi-rpm/commit/2e189391e4f9fbe6f86a1eed1c5f85d028168a0f

Output from gofed:
$ rpmlint heketi-1.0.0-1.fc22.src.rpm heketi-1.0.0-1.fc22.x86_64.rpm heketi-devel-1.0.0-1.fc22.noarch.rpm heketi-unit-test-devel-1.0.0-1.fc22.x86_64.rpm heketi-debuginfo-1.0.0-1.fc22.x86_64.rpm
heketi.src: W: invalid-url Source3: heketi-godeps-3f4a5b1.tar.gz
heketi.x86_64: W: non-standard-uid /var/lib/heketi heketi
heketi.x86_64: W: non-standard-gid /var/lib/heketi heketi
heketi.x86_64: W: no-manual-page-for-binary heketi
heketi.x86_64: W: no-manual-page-for-binary heketi-cli
5 packages and 0 specfiles checked; 0 errors, 5 warnings.

Oops I didn't provide the complete gofed output.  Here it is:
Spec URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi.spec

SRPM URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi-1.0.0-1.fc22.src.rpm

Description: Heketi provides a RESTful management interface which can be used to manage
the life cycle of GlusterFS volumes.  With Heketi, cloud services like
OpenStack Manila, Kubernetes, and OpenShift can dynamically provision
GlusterFS volumes with any of the supported durability types.  Heketi
will automatically determine the location for bricks across the cluster,
making sure to place bricks and its replicas across different failure
domains.  Heketi also supports any number of GlusterFS clusters, allowing
cloud services to provide network file storage without being limited to a
single GlusterFS cluster.

Fedora Account System Username: lpabon

$ rpmlint heketi-1.0.0-1.fc22.src.rpm heketi-1.0.0-1.fc22.x86_64.rpm heketi-devel-1.0.0-1.fc22.noarch.rpm heketi-unit-test-devel-1.0.0-1.fc22.x86_64.rpm heketi-debuginfo-1.0.0-1.fc22.x86_64.rpm
heketi.src: W: invalid-url Source3: heketi-godeps-3f4a5b1.tar.gz
heketi.x86_64: W: non-standard-uid /var/lib/heketi heketi
heketi.x86_64: W: non-standard-gid /var/lib/heketi heketi
heketi.x86_64: W: no-manual-page-for-binary heketi
heketi.x86_64: W: no-manual-page-for-binary heketi-cli
5 packages and 0 specfiles checked; 0 errors, 5 warnings.

Can you change
bundled(github.com/auth0/go-jwt-middleware)
to
bundled(golang(github.com/auth0/go-jwt-middleware))
for all bundled?

Short commit is fine as well. In future I will be using entire commit as it is then straightforward to download its tarball and run scans on it.

review is on so good luck :)

Sure, I can make that change.  Also, I can use the entire commit now also.  It is actually easier for me :-).

Ok, here is the latest:

Diff:
https://github.com/lpabon/heketi-rpm/commit/74a3fcfa46059c498d2032fc68645906a66565bd

Gofed:
Spec URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi.spec

SRPM URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi-1.0.0-1.fc22.src.rpm

Description: Heketi provides a RESTful management interface which can be used to manage
the life cycle of GlusterFS volumes.  With Heketi, cloud services like
OpenStack Manila, Kubernetes, and OpenShift can dynamically provision
GlusterFS volumes with any of the supported durability types.  Heketi
will automatically determine the location for bricks across the cluster,
making sure to place bricks and its replicas across different failure
domains.  Heketi also supports any number of GlusterFS clusters, allowing
cloud services to provide network file storage without being limited to a
single GlusterFS cluster.

Fedora Account System Username: lpabon

$ rpmlint heketi-1.0.0-1.fc22.src.rpm heketi-1.0.0-1.fc22.x86_64.rpm heketi-devel-1.0.0-1.fc22.noarch.rpm heketi-unit-test-devel-1.0.0-1.fc22.x86_64.rpm heketi-debuginfo-1.0.0-1.fc22.x86_64.rpm
heketi.src: W: invalid-url Source3: heketi-godeps-3f4a5b1.tar.gz
heketi.x86_64: W: non-standard-uid /var/lib/heketi heketi
heketi.x86_64: W: non-standard-gid /var/lib/heketi heketi
heketi.x86_64: W: no-manual-page-for-binary heketi
heketi.x86_64: W: no-manual-page-for-binary heketi-cli
5 packages and 0 specfiles checked; 0 errors, 5 warnings.

Good to go :)

heketi-1.0.0-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-8bb88f3e8a

heketi-1.0.0-1.el6 has been submitted as an update to Fedora EPEL 6. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-8ad0c47397

In future, I would like to suggest building epel7 golang projects purely from bundled deps. The current effort was to remove as many golang packages from epel7 as possible. As some projects can be added to RHEL7 extras causing the epel7 package to be obsoleted/removed. Just noting :).

heketi-1.0.0-1.el6 has been pushed to the Fedora EPEL 6 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'yum --enablerepo=epel-testing update heketi'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-8ad0c47397

heketi-1.0.0-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'yum --enablerepo=epel-testing update heketi'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-8bb88f3e8a

heketi-1.0.1-1.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-4c8a9e5f52

heketi-1.0.1-1.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-08f184de52

heketi-1.0.1-1.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update heketi'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-08f184de52

heketi-1.0.1-1.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update heketi'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-4c8a9e5f52

heketi-1.0.0-1.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

heketi-1.0.0-1.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.

heketi-1.0.1-1.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

heketi-1.0.1-1.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.