Bug 1185301 - Review Request: gnome-builder - IDE for writing GNOME-based software
Summary: Review Request: gnome-builder - IDE for writing GNOME-based software
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Igor Gnatenko
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-01-23 12:24 UTC by David King
Modified: 2015-01-23 17:57 UTC (History)
6 users (show)

Fixed In Version: gnome-builder-3.15.4.1-1.fc22
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-01-23 17:57:07 UTC
ignatenko: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description David King 2015-01-23 12:24:44 UTC
Spec URL: https://amigadave.fedorapeople.org/gnome-builder.spec
SRPM URL: https://amigadave.fedorapeople.org/gnome-builder-3.15.4.1-1.fc22.src.rpm
Description: Builder attempts to be an IDE for writing software for GNOME. It does not try to be a generic IDE, but one specialized for writing GNOME software.
Fedora Account System Username: amigadave

Comment 1 Yanko Kaneti 2015-01-23 13:00:20 UTC
gnome-builder.x86_64: E: invalid-appdata-file /usr/share/appdata/org.gnome.Builder.appdata.xml

$ appstream-util validate /usr/share/appdata/org.gnome.Builder.appdata.xml 
/usr/share/appdata/org.gnome.Builder.appdata.xml: FAILED:
• style-invalid         : Not enough <p> content before <ul>

minor:
gnome-builder.spec:6: W: mixed-use-of-spaces-and-tabs (spaces: line 6, tab: line 1)

Comment 2 David King 2015-01-23 13:42:24 UTC
Thanks for the review! The AppData guidelines suggest that "validate-relax" is sufficient (which is included in the spec, and passes locally), although it would of course be nice if there were a few more paragraphs. I fixed the tabs-and-spaces warning and reuploaded the spec and srpm in place.

Comment 3 Mathieu Bridon 2015-01-23 14:31:13 UTC
License tag is incorrect, if I checked correctly it should be:

    License:       GPLv3+ and GPLv2+ and LGPLv3+ and LGPLv2+ and MIT and CC-BY-SA and CC0

You could add a comment just above the license tag explaining what makes it so complex, like I had in the spec file for my Copr:

# Most of GNOME Builder is licensed under the GPLv3+.
#
# The following files are licensed under the GPLv2+:
#     - src/dialogs/gb-close-confirmation-dialog.c
#     - src/dialogs/gb-close-confirmation-dialog.h
#     - src/gedit/gedit-menu-stack-switcher.c
#     - src/gedit/gedit-menu-stack-switcher.h
#
# The following files are licensed under the LGPLv2+:
#     - data/styles/builder-dark.xml (seems like an error, the header speaks about GtkSourceView)
#     - src/animation/gb-frame-source.c
#     - src/animation/gb-frame-source.h
#     - src/fuzzy/fuzzy.c
#     - src/fuzzy/fuzzy.h
#     - src/gd/gd-tagged-entry.c
#     - src/gd/gd-tagged-entry.h
#     - src/gedit/gedit-close-button.c
#     - src/gedit/gedit-close-button.h
#     - src/nautilus/nautilus-floating-bar.c
#     - src/nautilus/nautilus-floating-bar.h
#     - src/trie/trie.c
#     - src/trie/trie.h
#     - src/util/gb-cairo.c
#     - src/util/gb-cairo.h
#     - src/util/gb-glib.h
#     - src/util/gb-gtk.c
#     - src/util/gb-gtk.h
#
# The following files are licensed under the LGPLv3+:
#     - src/auto-indent/gb-source-auto-indenter.c
#     - src/auto-indent/gb-source-auto-indenter.h
#     - src/auto-indent/gb-source-auto-indenter-c.c
#     - src/auto-indent/gb-source-auto-indenter-c.h
#     - src/auto-indent/gb-source-auto-indenter-python.c
#     - src/auto-indent/gb-source-auto-indenter-python.h
#     - src/auto-indent/gb-source-auto-indenter-xml.c
#     - src/auto-indent/gb-source-auto-indenter-xml.h
#
# The following files are MIT licensed:
#     - src/resources/css/markdown.css
#     - src/resources/js/marked.js
#
# The following files are licensed under the CC-BY-SA license:
#     - data/icons/
#
# The following files are licensed under the CC0 license:
#     - data/org.gnome.Builder.appdata.xml
#     - data/html-preview.png

Other than that, please use %license for the COPYING file, rather than %doc.

Finally, that #VCS comment seems wrong, unless you're packaging cheese again. :)

Comment 4 David King 2015-01-23 15:18:14 UTC
(In reply to Mathieu Bridon from comment #3)
> License tag is incorrect, if I checked correctly it should be:
>…

Thanks for that! I would normally trim this down a bit to the effective license of the binary, which in this case would allow dropping of the LGPLv2+ and GPLv2+ bits (as all the files under those licenses are built into a binary with GPLv3+ code, and effectively GPLv3+). However, this probably needs too much careful examination of licenses to maintain well.

In any case, fixed now.

> Other than that, please use %license for the COPYING file, rather than %doc.

Cool, this was not part of the licensing guidelines the last time I checked, so fixed too.

> Finally, that #VCS comment seems wrong, unless you're packaging cheese
> again. :)

Whoops! Fixed now.

Scratch build, including the changes, and a 32-bit build fix from upstream (also reuploaded in place):

http://koji.fedoraproject.org/koji/taskinfo?taskID=8707142

Comment 5 Igor Gnatenko 2015-01-23 15:19:09 UTC
directory /usr/share/icons/hicolor/scalable/autocomplete/ without known owner


E: invalid-appdata-file /usr/share/appdata/org.gnome.Builder.appdata.xml
interesting.

Comment 6 David King 2015-01-23 15:21:06 UTC
(In reply to Igor Gnatenko from comment #5)
> directory /usr/share/icons/hicolor/scalable/autocomplete/ without known owner

Ooh. That's not part of the icon theme, so is probably worth an upstream bug (and owning in this package)?

> E: invalid-appdata-file /usr/share/appdata/org.gnome.Builder.appdata.xml
> interesting.

Probably related to comment #1.

Comment 7 David King 2015-01-23 15:31:36 UTC
(In reply to David King from comment #6)
> (In reply to Igor Gnatenko from comment #5)
> > directory /usr/share/icons/hicolor/scalable/autocomplete/ without known owner
> 
> Ooh. That's not part of the icon theme, so is probably worth an upstream bug
> (and owning in this package)?

I owned the directory now, and reuploaded the fixed spec and srpm in place.

http://koji.fedoraproject.org/koji/taskinfo?taskID=8707271

Comment 8 Igor Gnatenko 2015-01-23 15:38:18 UTC
APPROVED.

Please add `ignatenkobrain` and `bochecha` to owners when you will create SCM request.

Comment 9 David King 2015-01-23 15:41:20 UTC
New Package SCM Request
=======================
Package Name: gnome-builder
Short Description: IDE for writing GNOME-based software
Upstream URL: https://wiki.gnome.org/Apps/Builder
Owners: amigadave bochecha ignatenkobrain
Branches:
InitialCC:

Comment 10 Mathieu Bridon 2015-01-23 15:47:41 UTC
Don't add me as owner, add group::gnome-sig ;)

Comment 11 David King 2015-01-23 15:50:38 UTC
New Package SCM Request
=======================
Package Name: gnome-builder
Short Description: IDE for writing GNOME-based software
Upstream URL: https://wiki.gnome.org/Apps/Builder
Owners: amigadave group::gnome-sig ignatenkobrain
Branches:
InitialCC:

Comment 12 Gwyn Ciesla 2015-01-23 15:57:27 UTC
Git done (by process-git-requests).

Comment 13 David King 2015-01-23 17:57:07 UTC
Thanks for the reviews!


Note You need to log in before you can comment on or make changes to this bug.