Bug 1128378 - Review Request: cv - Coreutils Viewer
Summary: Review Request: cv - Coreutils Viewer
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Christopher Meng
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-08-09 19:29 UTC by Florian "der-flo" Lehner
Modified: 2015-08-18 05:19 UTC (History)
5 users (show)

Fixed In Version: cv-0.4.1-4.fc20
Clone Of:
Environment:
Last Closed: 2014-08-23 01:55:54 UTC
Type: ---
Embargoed:
i: fedora-review+
dev: needinfo+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Florian "der-flo" Lehner 2014-08-09 19:29:58 UTC
Spec URL: https://flo.fedorapeople.org/cv.spec
SRPM URL: https://flo.fedorapeople.org/cv-0.4.1-1.fc20.src.rpm
Description: Coreutils Viewer can be described as a Tiny Dirty Linux Only* C command that looks for coreutils basic commands (cp, mv, dd, tar, gzip/gunzip, cat, ...) currently running on your system and displays the percentage of copied data.

Fedora Account System Username: flo

koji-Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=7268342

Comment 1 Christopher Meng 2014-08-10 02:13:05 UTC
So fast ;) I talked to some openSUSE guys last week about this package.

As I want to notify here, upstream has adopted a manpage. So I hope you can package a snapshot here.

Comment 2 Christopher Meng 2014-08-10 02:22:41 UTC
You are building it without honoring the flags defined already.

See my PR also: https://github.com/Xfennec/cv/pull/29

Comment 3 Florian "der-flo" Lehner 2014-08-10 08:58:06 UTC
hi!

Thanks for assigning!

I know there is a patch merged already for a manpage. But it's not part of the current stable version. I am quite sure there will be a new release containing this manpage.

I will wait and see what happens upstream with your PR.

Instead of overriding the flags I appended the fedora-default flags. So there is a new version:

Spec URL: https://flo.fedorapeople.org/cv.spec
SRPM URL: https://flo.fedorapeople.org/cv-0.4.1-2.fc20.src.rpm
koji-Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=7269524

Comment 4 Ralf Corsepius 2014-08-10 09:30:33 UTC
Some remarks:

1. I do not see any reason why this package should be built hardened.
=> Remove "%global _hardened_build 1"

2. Passing LDFLAGS is not required.
=> Remove LDFLAGS+="%{?__global_ldflags}" 

3.  Package does not honor CFLAGS:

gcc -g -Wall -D_FILE_OFFSET_BITS=64 -c cv.c
gcc -g -Wall -D_FILE_OFFSET_BITS=64 -c sizes.c
gcc -g -Wall -D_FILE_OFFSET_BITS=64 -c hlist.c
gcc -Wall cv.o sizes.o hlist.o -o cv -lncurses -lm  

This package uses simple and naive Makefile, so you need to override CFLAGS from the environment or make's command-line.

Comment 5 Björn Esser (besser82) 2014-08-10 09:44:29 UTC
(In reply to Ralf Corsepius from comment #4)

> 2. Passing LDFLAGS is not required.
> => Remove LDFLAGS+="%{?__global_ldflags}" 

Ralf are you serious?!?  Why are those LDFLAGS setup by %configure or %cmake macros then, if they are not needed?!?

Passing those LDFLAGS is important to have at least partial RELRO applied onto the linked binaries:

$ rpm --eval %__global_ldflags
-Wl,-z,relro  ---> results in partial relro binaries

and when applying hardening flags:

$ rpm -D'_hardened_build 1' --eval %__global_ldflags
-Wl,-z,relro -specs=/usr/lib/rpm/redhat/redhat-hardened-ld  ---> will result in in PIE / PIC and fully relro binaries.

So please explain why LDFLAGS are not required and should be removed!?!

Comment 6 Christopher Meng 2014-08-10 11:09:55 UTC
(In reply to Ralf Corsepius from comment #4)
> 2. Passing LDFLAGS is not required.
> => Remove LDFLAGS+="%{?__global_ldflags}" 

No. It's linked against ncurses. It's a MUST.

(In reply to Florian "der-flo" Lehner from comment #3)
> Instead of overriding the flags I appended the fedora-default flags.

Obviously You don't know how to use automake IMO. You are still overriding the flags.

Comment 7 Christopher Meng 2014-08-10 11:10:28 UTC
If you don't use my patch, simply hack from command line won't work.

Comment 8 Florian "der-flo" Lehner 2014-08-10 13:10:24 UTC
ok - here we are again.

I hope I have understood you and implemented the points correctly.

Spec URL: https://flo.fedorapeople.org/cv.spec
SRPM URL: https://flo.fedorapeople.org/cv-0.4.1-3.fc20.src.rpm
koji-Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=7270227

Comment 9 Christopher Meng 2014-08-11 02:16:29 UTC
1. Please, unset %global _hardened_build 1, I don't know why you prefer adding this to all packages.

2. LDFLAGS still not set:

cc -Wall cv.o sizes.o hlist.o -o cv -lncurses -lm

In your spec:

LDFLAGS="%{?__global_ldflags}"	\

Ooops, upstream uses LFLAGS XD.

3. mkdir -p %{buildroot}%{_bindir}
install -pm 0755 %{name} %{buildroot}%{_bindir}/%{name}

If you read the makefile carefully, you can try "%make_install PREFIX=%{buildroot}%{_prefix}". In the next version, use "%make_install PREFIX=%{_prefix}".

Comment 10 Florian "der-flo" Lehner 2014-08-11 17:38:46 UTC
Long working days are long. So here we go:

I've done what you said.

(In reply to Christopher Meng from comment #9)
> If you read the makefile carefully, you can try "%make_install
> PREFIX=%{buildroot}%{_prefix}". In the next version, use "%make_install
> PREFIX=%{_prefix}".

Looking at your PR the Makefile will change in future versions. That's why I've chosen this way to install the binary at the moment. In the next version I will use your proposal.

Spec URL: https://flo.fedorapeople.org/cv.spec
SRPM URL: https://flo.fedorapeople.org/cv-0.4.1-4.fc20.src.rpm
koji-Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=7274442

Comment 11 Christopher Meng 2014-08-12 02:55:15 UTC
PACKAGE APPROVED.

Comment 12 Florian "der-flo" Lehner 2014-08-12 04:06:27 UTC
Thanks for reviewing!

Comment 13 Christopher Meng 2014-08-12 07:06:48 UTC
You forgot to fill the SCM form.

Comment 14 Gwyn Ciesla 2014-08-12 12:13:19 UTC
No SCM request found.

Comment 15 Florian "der-flo" Lehner 2014-08-12 15:12:07 UTC
New Package SCM Request
=======================
Package Name: cv
Short Description: Coreutils Viewer
Upstream URL: https://github.com/Xfennec/cv
Owners: flo
Branches: f19 f20 f21 el6 epel7
InitialCC:

Comment 16 Gwyn Ciesla 2014-08-12 15:41:15 UTC
Git done (by process-git-requests).

Comment 17 Ralf Corsepius 2014-08-12 16:16:16 UTC
(In reply to Christopher Meng from comment #6)
> (In reply to Ralf Corsepius from comment #4)
> > 2. Passing LDFLAGS is not required.
> > => Remove LDFLAGS+="%{?__global_ldflags}" 
> 
> No. It's linked against ncurses. It's a MUST.
Provide a pointer to the FPG, which requires to set __global_ldflags when using ncurses.
 
> (In reply to Florian "der-flo" Lehner from comment #3)
> > Instead of overriding the flags I appended the fedora-default flags.
> 
> Obviously You don't know how to use automake IMO. You are still overriding
> the flags.
This has nothing to do with automake. It's a case of a crappy Makefile.

Comment 18 Fedora Update System 2014-08-12 18:38:56 UTC
cv-0.4.1-4.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/cv-0.4.1-4.fc19

Comment 19 Fedora Update System 2014-08-12 18:51:07 UTC
cv-0.4.1-4.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/cv-0.4.1-4.fc20

Comment 20 Fedora Update System 2014-08-15 02:30:14 UTC
cv-0.4.1-4.fc20 has been pushed to the Fedora 20 testing repository.

Comment 21 Fedora Update System 2014-08-23 01:55:54 UTC
cv-0.4.1-4.fc19 has been pushed to the Fedora 19 stable repository.

Comment 22 Fedora Update System 2014-08-23 01:56:36 UTC
cv-0.4.1-4.fc20 has been pushed to the Fedora 20 stable repository.

Comment 23 Christopher Meng 2015-08-18 05:19:51 UTC
I doubt if upstream will change name again, now more and more upstream have simple brain and can't came up with any name longer than 3.

Now it's "progress".


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