Bug 1168692 - Review Request: zram-swap - Enable compressed swap in memory
Summary: Review Request: zram-swap - Enable compressed swap in memory
Keywords:
Status: CLOSED DUPLICATE of bug 1179835
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michel Alexandre Salim
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-11-27 15:39 UTC by Juan Orti Alcaine
Modified: 2015-01-07 16:09 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-01-07 16:09:25 UTC
michel: fedora-review?


Attachments (Terms of Use)

Description Juan Orti Alcaine 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 Alexandre Salim 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 Alcaine 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 Alexandre Salim 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 Alcaine 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 ***


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