Bug 1005504

Summary: Review Request: etcd - key value store package.
Product: [Fedora] Fedora Reporter: Luke Cypret <lacypret>
Component: Package ReviewAssignee: Peter Lemenkov <lemenkov>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: besser82, davdunc, lemenkov, lsm5, notting, quantum.analyst, tdawson
Target Milestone: ---Flags: lemenkov: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: etcd-0.1.1-1.fc19 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-09-18 15:50:10 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Luke Cypret 2013-09-07 17:23:06 UTC
Spec URL: http://ec2-54-227-53-121.compute-1.amazonaws.com/etcd.spec
SRPM URL: http://ec2-54-227-53-121.compute-1.amazonaws.com/etcd-v0.1.1-1.fc19.src.rpm
Description: A highly-available key value store for shared configuration and service discovery.
Fedora Account System Username: cypret

Comment 1 Björn 'besser82' Esser 2013-09-07 17:26:10 UTC
taken ;)

Comment 2 Peter Lemenkov 2013-09-08 14:32:33 UTC
Initial comments.

* Version field is wrong. You must use 0.1.1 (not "v0.1.1").

* You must mark LICENSE as %doc. Also you installed doc-files into versioned doc-dir which is wrong since Fedora 20. I advise you to remove this line:

cp Documentation/* $RPM_BUILD_ROOT/usr/share/doc/%{name}-%{version}

and simply mark necessary files as %doc. E.g.

%doc LICENSE README.md Documentation/internal-protocol-versioning.md

Comment 3 Luke Cypret 2013-09-08 18:30:02 UTC
Thanks, new SRPM location since the naming was off for the original submitted:
http://ec2-54-227-53-121.compute-1.amazonaws.com/etcd-0.1.1-1.fc19.src.rpm

Files have been updated, your recommendation of marking those files simply as %doc was much easier compared to how I originally set it up.

Comment 4 Peter Lemenkov 2013-09-09 05:38:38 UTC
(In reply to Luke Cypret from comment #3)
> Thanks, new SRPM location since the naming was off for the original
> submitted:
> http://ec2-54-227-53-121.compute-1.amazonaws.com/etcd-0.1.1-1.fc19.src.rpm
> 
> Files have been updated, your recommendation of marking those files simply
> as %doc was much easier compared to how I originally set it up.

Ok, good. (Un)fortunately I've got few more advices:

* No need to create directories in %install section. E.g. you may drop these two lines:

install -m 755 -d %{buildroot}%{_bindir}
install -m 755 -d %{buildroot}%{_docdir}

* When you remove these lines, please use more command-line switches while installing binary:

install -D -p  etcd %{buildroot}%{_bindir}/etcd

-D means that install should ensure that all necessary directories are available and create them if necessary (so you may remove one of the directory creation commands listed above)
-p means "preserve timestamps". I don't see much point in this (except the case when ypu're installing pre-built files, such as HTML pages, pictures, fonts, etc), but others sometimes are very insistent about that. It doesn't hurt so why not to add and make those fellow Fedora maintainers happy?

* In the %files section please replace explicit /usr/bin with %{_bindir} - just for consistency with %install section.

* Source0 path is definitely unacceptable. You must point to the official tarball, either from GitHub or from somewhere else.I propose the following address:

Source0: https://github.com/coreos/%{name}/archive/v%{version}/%{name}-v%{version}.tar.gz

(Note that version is prefixed with "v", since it's the part of a git tag).

Comment 5 Peter Lemenkov 2013-09-10 06:13:26 UTC
Ping, Luke! How's it going?

Comment 6 Luke Cypret 2013-09-10 14:23:16 UTC
All good aside from getting hung on the naming scheme for the dir and new source marked as .tar.gz.

I'll have the new updates in <10 hours.

Comment 7 Luke Cypret 2013-09-10 16:55:23 UTC
I've got my latest updated:
http://ec2-54-227-53-121.compute-1.amazonaws.com/etcd.spec
http://ec2-54-227-53-121.compute-1.amazonaws.com/etcd-0.1.1-1.fc19.src.rpm

The reason I specified the other source instead of github has to do with the way tags work. I based this build off of the tag v0.1.1 which doesn't have a specific download URL other than https://github.com/coreos/etcd/tree/v0.1.1 for when that version was first tagged. Therefor for the source I had to clone etcd then 'checkout tags/v0.1.1'.

I figured it would be best to use the files from that specific time it was tagged since there are still new commits being pushed for v0.1.1.

Comment 9 Luke Cypret 2013-09-10 17:32:24 UTC
$ rpmlint etcd.spec 
etcd.spec: W: invalid-url Source0: https://github.com/coreos/etcd/archive/etcd-v0.1.1.tar.gz HTTP Error 404: Not Found

Comment 10 Peter Lemenkov 2013-09-17 10:38:19 UTC
Koji scratchbuild for F19:

* http://koji.fedoraproject.org/koji/taskinfo?taskID=5944557

REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

- rpmlint is NOT silent

work ~: rpmlint Desktop/etcd-0.1.1-1.fc19.*
etcd.src: W: invalid-url Source0: https://github.com/coreos/etcd/archive/etcd-v0.1.1.tar.gz HTTP Error 404: Not Found

^^^ You forgot to add git tag between "archive" and tarball-name. E.g. proper link should be:

* https://github.com/coreos/%{name}/archive/v%{version}/%{name}-v%{version}.tar.gz

etcd.x86_64: W: unstripped-binary-or-object /usr/bin/etcd

^^^ That's ok for now. We still don't have golang packaging guidelines.

etcd.x86_64: W: no-manual-page-for-binary etcd

^^^ That's sad but that's an upstream issue, not ours.

2 packages and 0 specfiles checked; 0 errors, 3 warnings.
work ~:

So please fix Source0 link again.

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines (note that we in Fedora don't have golang-specific ones yet).
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license (Apache Software License ver. 2.0).
+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the spec URL.

sulaco ~/rpmbuild/SOURCES: sha256sum etcd-v0.1.1.tar.gz*
e2d48a1584a21626ff19e0513ba2be38a98c3290c1b6072b7cf9dd6ff46a751a  etcd-v0.1.1.tar.gz
e2d48a1584a21626ff19e0513ba2be38a98c3290c1b6072b7cf9dd6ff46a751a  etcd-v0.1.1.tar.gz.1
sulaco ~/rpmbuild/SOURCES:

+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See  Koji link above.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files in some of the dynamic linker's default paths.
+ The package does NOT bundle copies of system libraries.
0 The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No C/C++ header files.
0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package doesn't contain library files without a suffix (e.g. libfoo.so) in some of the dynamic linker's default paths.
0 No devel sub-package.
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ All filenames in rpm packages are valid UTF-8.

OK, so please 

* fix Source0, 
* remove .fc19 from %changelog entry

And I'll finish this.

Comment 11 Peter Lemenkov 2013-09-17 10:40:40 UTC
Unblocking FE-NEEDSPONSOR - I've just sponsored Luke.

Comment 12 Luke Cypret 2013-09-17 16:29:10 UTC
Hi Peter, the new changes have been published.

The git naming convention with tags was a little confusing but a good learning experience.

Comment 13 Peter Lemenkov 2013-09-17 16:34:48 UTC
Ok, good. I don't see any other issues so this package is


APPROVED.


Luke, continue with the following steps:

* https://fedoraproject.org/wiki/Package_SCM_admin_requests#New_Packages

Comment 14 Luke Cypret 2013-09-17 21:32:56 UTC
New Package SCM Request
=======================
Package Name: etcd
Short Description: A highly-available key value store for shared configuration
Owners: cypret
Branches: f19 f20
InitialCC: lemenkov

Comment 15 Gwyn Ciesla 2013-09-18 12:23:57 UTC
Git done (by process-git-requests).

Left off lemenkov, not a valid FAS account, can be corrected later in pkgdb.

Comment 16 Luke Cypret 2013-09-18 15:50:10 UTC
Imported to FC21, FC20, FC19 and built successfully in each branch.

Comment 17 Fedora Update System 2013-09-18 15:57:37 UTC
etcd-0.1.1-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/etcd-0.1.1-1.fc19

Comment 18 Fedora Update System 2013-09-18 16:00:20 UTC
etcd-0.1.1-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/etcd-0.1.1-1.fc20

Comment 19 Fedora Update System 2013-09-27 00:30:04 UTC
etcd-0.1.1-1.fc20 has been pushed to the Fedora 20 stable repository.

Comment 20 Fedora Update System 2013-09-27 00:48:31 UTC
etcd-0.1.1-1.fc19 has been pushed to the Fedora 19 stable repository.

Comment 21 Peter Lemenkov 2013-10-23 11:13:14 UTC
Package Change Request
======================
Package Name: etcd
InitialCC: golang-sig

Comment 22 Gwyn Ciesla 2013-10-23 14:03:13 UTC
Done.

Comment 23 Lokesh Mandvekar 2013-10-26 20:12:09 UTC
Peter, Luke:

Could you let me know if all golang deps are good to go to build etcd, or if there's something I missed?

Comment 24 Peter Lemenkov 2013-10-27 05:37:21 UTC
(In reply to Lokesh Mandvekar from comment #23)
> Peter, Luke:
> 
> Could you let me know if all golang deps are good to go to build etcd, or if
> there's something I missed?

Yes, it's perfectly fine. Next etcd release will probably depend on a couple of a new libs btw, but this one works good.

Comment 25 Troy Dawson 2014-07-22 19:17:29 UTC
Package Change Request
======================
Package Name: etcd
New Branches: epel7
Owners: tdawson

Comment 26 Gwyn Ciesla 2014-07-22 19:35:07 UTC
Git done (by process-git-requests).