[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065

Miroslav Suchý  changed:

   What|Removed |Added

 Blocks|177841 (FE-NEEDSPONSOR) |




Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://lists.fedoraproject.org/admin/lists/package-review@lists.fedoraproject.org

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #16 from Fedora Update System  ---
hexer-0.2.3-1.fc24 has been pushed to the Fedora 24 stable repository. If
problems still persist, please make note of it in this bug report.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://lists.fedoraproject.org/admin/lists/package-review@lists.fedoraproject.org

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065

Fedora Update System  changed:

   What|Removed |Added

 Status|ON_QA   |CLOSED
 Resolution|--- |ERRATA
Last Closed||2016-07-02 11:29:35



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://lists.fedoraproject.org/admin/lists/package-review@lists.fedoraproject.org

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065

Fedora Update System  changed:

   What|Removed |Added

 Status|MODIFIED|ON_QA



--- Comment #15 from Fedora Update System  ---
hexer-0.2.3-1.fc24 has been pushed to the Fedora 24 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-2016-7d69fa003b

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #14 from Fedora Update System  ---
hexer-0.2.3-1.fc24 has been submitted as an update to Fedora 24.
https://bodhi.fedoraproject.org/updates/FEDORA-2016-7d69fa003b

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065

Fedora Update System  changed:

   What|Removed |Added

 Status|POST|MODIFIED



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #13 from jiri vanek  ---
(In reply to Alex Kashchenko from comment #12)
> Thanks for the review, on the comments:
> 
> > There are sources of some "calc" even with manpage. Is it intentionally 
> > not-packed?
> 
> Yes, it is a separate utility myc that is not built by default. It's code is
> also built into the main hexer binary.

I see. If you will ever pack it, I would recommend to prefix it to hexer-calc
(And same wit manpage)

But its completely up to you.

if you don't mind, I will comaintaint this package with you and observe the
changes you are doing for some time.
> 
> > It seems that makefile is supporting setupable bindir and mandir. Maybe it 
> > is better to set them in make and make install rather then cp?
> 
> "install" target depends on "all" target in that Makefile - so it is not
> convenient to use it inside the "%install". As only two files are required
> to be copied, current way looks more straightforward.

np.

> 
> > Have you tried arm32 build?
> 
> Yes, armv7hl build works.

thanx:)
> 
> > One note, where I don't know how the world is standing - Have you 
> > considered moving .h files to devel subpackage? I guess there is no need to 
> > do it, unles syou really can use it as library.
> 
> It looks like it is not intended to be used as a library.

Sure.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #12 from Alex Kashchenko  ---
Thanks for the review, on the comments:

> There are sources of some "calc" even with manpage. Is it intentionally 
> not-packed?

Yes, it is a separate utility myc that is not built by default. It's code is
also built into the main hexer binary.

> It seems that makefile is supporting setupable bindir and mandir. Maybe it is 
> better to set them in make and make install rather then cp?

"install" target depends on "all" target in that Makefile - so it is not
convenient to use it inside the "%install". As only two files are required to
be copied, current way looks more straightforward.

> Have you tried arm32 build?

Yes, armv7hl build works.

> One note, where I don't know how the world is standing - Have you considered 
> moving .h files to devel subpackage? I guess there is no need to do it, unles 
> syou really can use it as library.

It looks like it is not intended to be used as a library.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065

jiri vanek  changed:

   What|Removed |Added

 Status|ASSIGNED|POST
  Flags|fedora-review?  |fedora-review+



--- Comment #11 from jiri vanek  ---
This package is APPROVED.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #10 from jiri vanek  ---
There are sources of some "calc" even with manpage. Is it intentionally
not-packed?
It seems that makefile is supporting setupable bindir and mandir. Maybe it is
better to set them in make and make install rather then cp?
Have you tried arm32 build?

One note, where I don't know how the world is standing - Have you considered
moving .h files to devel subpackage? I guess there is no need to do it, unles
syou really can use it as library.

Anyway the package is ok from me.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #9 from jiri vanek  ---
Package Review
==

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



= MUST items =

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
 other legal requirements as defined in the legal section of Packaging
 Guidelines.
[x]: License field in the package spec file matches the actual license.
 Note: Checking patched sources after %prep for licenses. Licenses
 found: "BSD (3 clause)", "Unknown or generated". 7 files have unknown
 license. Detailed output of licensecheck in
 /home/jvanek/1213065-hexer/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
 names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
 Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
 (~1MB) or number of files.
 Note: Documentation size is 10240 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
 one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
 Note: No rpmlint messages.
[x]: If (and only if) the source package includes the text of the
 license(s) in its own file, then that file, containing the text of the
 license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
 that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
 beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
 work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
 provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
 %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

= SHOULD items =

Generic:
[!]: If the source package does not include license text(s) as a separate
 file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[-]: Fully versioned dependency in subpackages if applicable.
 Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in hexer-
 debuginfo
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
 translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
 architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
 files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer 

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #7 from Alex Kashchenko  ---
Updated package to upstream version 0.2.3:

Spec URL: https://bitbucket.org/akasko/share/downloads/hexer.spec
SRPM URL:
https://bitbucket.org/akasko/share/downloads/hexer-0.2.3-1.fc24.src.rpm

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=13378191

RPMLint output:

rpmlint SRPMS/hexer-0.2.3-1.fc24.src.rpm 
> 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
rpmlint RPMS/x86_64/hexer-0.2.3-1.fc24.x86_64.rpm 
> 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
rpmlint RPMS/x86_64/hexer-debuginfo-0.2.3-1.fc24.x86_64.rpm 
> 1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comments addressed:

1) Release tag: In this update changed the upstream version, will increase
Release for the subsequent changes

2) License BSD: license name was taken from this table -
https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Good_Licenses ,
please let me know if any changes are required here

3) compiler flags: added export CFLAGS="%{optflags}" before calling make

4) debuginfo: in 0.2.3-1 debuginfo RPM passes rpmlint

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065

jiri vanek  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED



--- Comment #8 from jiri vanek  ---
Hello!

I'm runnign the review now. The apckage is looking more then good.

The most painful issue are remaining the license and code.

Indeed the license in files and in copyright looks like BSD. And they probably
are. Also the tarball provided as source tarball looks like open sources :)
I' would like to see the license more clearly, but from my side, I'm happy with
current state of things. Also I would like to see some publicly accessible
repository... But the open release tarball looks good enough for me.

Have I overlooked something?

I will publish final review soon.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065

jiri vanek  changed:

   What|Removed |Added

 CC||jva...@redhat.com
   Assignee|nob...@fedoraproject.org|jva...@redhat.com
  Flags||fedora-review?



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #6 from Upstream Release Monitoring 
 ---
akasko's scratch build of hexer-0.1.8-1.fc23.src.rpm for f23 completed
http://koji.fedoraproject.org/koji/taskinfo?taskID=12184492

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #5 from Michael Schwendt (Fedora Packager Sponsors Group) 
bugs.mich...@gmx.net ---
Try pointing the fedora-review tool at this ticket: fedora-review -b 1213065
It evaluates the Spec URL: and SRPM URL: lines, downloads the latest
packages, performs local test-builds and many checks related to the packaging
guidelines.


 please remove _v1 postfix after download

The better choice would have been to practice increasing Release for each
package update:

  https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes

 | Increase the Release tag every time you upload a new package to avoid
 | confusion. The reviewer and other interested parties probably still have 
 | older versions of your SRPM lying around to check what has changed between
 | the old and new packages; those get confused when the revision didn't 
 | change. 


 License: BSD

A modified 3-clause BSD that adds a specific requirement for documenting
changes in the README and the source files.

Not really a hurdle for the current patch but can be easy to miss.


 cc -O -DHEXER_VERSION=\0.1.8\  -c -o buffer.o buffer.c

https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

rpmlint:
hexer-debuginfo.x86_64: E: debuginfo-without-sources

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #2 from Pranav Kant pranav...@gmail.com ---
There is a superfluous space before the start of the changelog entry.

* Fri Apr 17 2015 Alex Kashchenko m...@alexkasko.com 0.1.8-1
 - initial package, in sigaction struct sa_mask field is zeroed using
   sigemptyset function instead of assigning zero (prohibited in gcc5),
   upstream changelog link: http://devel.ringlet.net/editors/hexer/CHANGES

Space before '-'

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #3 from Pranav Kant pranav...@gmail.com ---
rm -rf %{buildroot} at the beginning of the install is not required. You can
remove it.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065

Pranav Kant pranav...@gmail.com changed:

   What|Removed |Added

 CC||pranav...@gmail.com



--- Comment #1 from Pranav Kant pranav...@gmail.com ---
These are unofficial review comments.

You referred to this link :

http://devel.ringlet.net/editors/hexer/CHANGES

explaining the reason of your patch, but the file in this link is not updated,
and doesn't have any changes mentioned for version 0.1.8

Moreover, explaining the reason for your patch near Patch0 field would be
better, in my humble opinion.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065



--- Comment #4 from Alex Kashchenko m...@alexkasko.com ---
Addressed comments 1, 2 and 3 (please remove _v1 postfix after download):

Spec URL: https://bitbucket.org/akasko/share/downloads/hexer.spec_v1
SRPM URL:
https://bitbucket.org/akasko/share/downloads/hexer-0.1.8-1.fc23.src.rpm_v1

Diff between v0 and v1 specs:
https://bitbucket.org/akasko/share/downloads/v1_spec.diff


Comment 1:

 - removed upstream CHANGES link because this link
(http://devel.ringlet.net/editors/hexer/CHANGES) is not up to date, and while
up to date link exists in VSC
(https://www.gitorious.org/hexer/hexer/source/ed78162928ba92dc455e5bf1a3d05914bd993bae:CHANGES),
it is going to become unavailable soon after gitorius.org shutdown
 - moved patch description next to Patch0 entry as a comment
 - added the comment about patch upstreaming

Comment 2:

 - removed superfluous space in changelog

Comment 3:

 - removed rm -rf %{buildroot} entry


RPMLint output:

rpmlint hexer-0.1.8-1.fc23.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
rpmlint hexer-0.1.8-1.fc23.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1213065] Review Request: hexer - ncurses-based binary editor

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1213065

Alex Kashchenko m...@alexkasko.com changed:

   What|Removed |Added

 Blocks||177841 (FE-NEEDSPONSOR)




Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review