Bug 1005504
Summary: | Review Request: etcd - key value store package. | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Luke Cypret <lacypret> |
Component: | Package Review | Assignee: | Peter Lemenkov <lemenkov> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | 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
taken ;) 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 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. (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). Ping, Luke! How's it going? 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. 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. See what I mean with the URLs and file names: https://github.com/coreos/etcd/archive/v0.1.1.tar.gz https://github.com/coreos/etcd/tree/v0.1.1 Source0: https://github.com/coreos/%{name}/archive/%{name}-v%{version}.tar.gz $ 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 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. Unblocking FE-NEEDSPONSOR - I've just sponsored Luke. Hi Peter, the new changes have been published. The git naming convention with tags was a little confusing but a good learning experience. 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 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 Git done (by process-git-requests). Left off lemenkov, not a valid FAS account, can be corrected later in pkgdb. Imported to FC21, FC20, FC19 and built successfully in each branch. 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 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 etcd-0.1.1-1.fc20 has been pushed to the Fedora 20 stable repository. etcd-0.1.1-1.fc19 has been pushed to the Fedora 19 stable repository. Package Change Request ====================== Package Name: etcd InitialCC: golang-sig Done. 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? (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. Package Change Request ====================== Package Name: etcd New Branches: epel7 Owners: tdawson Git done (by process-git-requests). |