Bug 1168692

Summary:	Review Request: zram-swap - Enable compressed swap in memory
Product:	[Fedora] Fedora	Reporter:	Juan Orti <jorti>
Component:	Package Review	Assignee:	Michel Lind <michel>
Status:	CLOSED DUPLICATE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	amigo.elite, gilboad, jorti, michel, package-review
Target Milestone:	---	Flags:	michel: fedora-review?
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2015-01-07 16:09:25 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 Juan Orti 2014-11-27 15:39:04 UTC

Spec URL: https://jorti.fedorapeople.org/zram-swap/zram-swap.spec
SRPM URL: https://jorti.fedorapeople.org/zram-swap/zram-swap-1.0.2-1.fc21.src.rpm
Description: zram compresses swap partitions into RAM for performance
Fedora Account System Username: jorti

Comment 1 Michel Lind 2014-11-28 09:53:33 UTC

Taking this review

Comment 2 Vladimir Stackov 2014-12-16 09:12:22 UTC

Greetings,

a few comments:

1. Why not to push latest SPEC to your pull-request?
I mean this: https://github.com/mystilleef/FedoraZram/pull/12/files
It could be merged and then you can use upstream SOURCE.

2. You could easily remove empty %build from your SPEC.

3. Package name is misleading. It's not related to zswap so you should probably rename it to something like zramctl.

4. Why you don't want to include zramstat? It was very useful util.

5. Why you decided to use manual install instead of %makeinstall?

Thanks!

Please note that this is informal review.

Comment 3 Juan Orti 2014-12-18 11:11:40 UTC

(In reply to Vladimir Stackov from comment #2)
> Greetings,
> 
> a few comments:
> 
> 1. Why not to push latest SPEC to your pull-request?
> I mean this: https://github.com/mystilleef/FedoraZram/pull/12/files
> It could be merged and then you can use upstream SOURCE.

That is another repo. I took the scripts from https://lists.fedoraproject.org/pipermail/devel/2014-November/204684.html

> 
> 2. You could easily remove empty %build from your SPEC.

rpmlint warns against that

> 
> 3. Package name is misleading. It's not related to zswap so you should
> probably rename it to something like zramctl.

The script creates a swap on zram, so I think zram-swap fits quite well.

> 
> 4. Why you don't want to include zramstat? It was very useful util.

I see that script is from https://github.com/mystilleef/FedoraZram , I could merge it, but we are talking about other repo again.

> 
> 5. Why you decided to use manual install instead of %makeinstall?

There is no makefile

Comment 4 Vladimir Stackov 2014-12-18 13:39:22 UTC

(In reply to Juan Orti from comment #3)
> (In reply to Vladimir Stackov from comment #2)
> > Greetings,
> > 
> > a few comments:
> > 
> > 1. Why not to push latest SPEC to your pull-request?
> > I mean this: https://github.com/mystilleef/FedoraZram/pull/12/files
> > It could be merged and then you can use upstream SOURCE.
> 
> That is another repo. I took the scripts from
> https://lists.fedoraproject.org/pipermail/devel/2014-November/204684.html
So maby you should follow https://fedoraproject.org/wiki/Staying_close_to_upstream_projects?
lists.fedoraproject.org isn't upstream.

> 
> > 
> > 2. You could easily remove empty %build from your SPEC.
> 
> rpmlint warns against that
Ok, it's all up to you :)

> 
> > 
> > 4. Why you don't want to include zramstat? It was very useful util.
> 
> I see that script is from https://github.com/mystilleef/FedoraZram , I could
> merge it, but we are talking about other repo again.
What do you mean when you say "other repo"? It was upstream.

> 
> > 
> > 5. Why you decided to use manual install instead of %makeinstall?
> 
> There is no makefile
Then it's your chance to use upstream repo :)
https://github.com/mystilleef/FedoraZram/blob/master/Makefile

Comment 5 Michel Lind 2015-01-06 04:22:45 UTC

The URL seems wrong, and there is no mention of the license anywhere in the source. I'd indeed recommend using the mystilleef repo as your source

Comment 6 Juan Orti 2015-01-07 16:09:25 UTC

Ok, I accept to use mystilleef as the source. I have submitted a new review request at https://bugzilla.redhat.com/show_bug.cgi?id=1179835

*** This bug has been marked as a duplicate of bug 1179835 ***