Bug 657577 - Review Request: sdlhack - Force full-screen games to minimize
Summary: Review Request: sdlhack - Force full-screen games to minimize
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Volker Fröhlich
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-11-26 15:04 UTC by Hicham HAOUARI
Modified: 2010-12-30 20:22 UTC (History)
4 users (show)

Fixed In Version: sdlhack-1.1-1.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-12-30 20:20:25 UTC
volker27: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Hicham HAOUARI 2010-11-26 15:04:30 UTC
Spec URL: http://hicham.fedorapeople.org/sdlhack/sdlhack.spec
SRPM URL: http://hicham.fedorapeople.org/sdlhack/sdlhack-1.1-1.fc14.src.rpm
Description:
SDLHack is a wrapper for SDL which lets you force full-screen games to minimize.
It also allows you to disable joystick detection.

Comment 1 Volker Fröhlich 2010-12-19 14:37:39 UTC
I assume, the hack is useless without SDL installed. Please require SDL.

You may consider to build and install explicitly, since it requires about as many lines as the sed approach does.

Writing "./build" and "./install" should be fine as well.

Please give a rationale, why you choose to create an own directory for the library to install to.

You can simplify the files section to:

%{_bindir}/*
%{_libdir}/%{name}
%{_mandir}/man1/*

I don't know if this package makes sense for EPEL. If you don't want to introduce it there, you can drop the buildroot, the clean section and the rm for buildroot in the install section.

Comment 2 Hicham HAOUARI 2010-12-19 16:05:17 UTC
(In reply to comment #1)
> I assume, the hack is useless without SDL installed. Please require SDL.
> 
> You may consider to build and install explicitly, since it requires about as
> many lines as the sed approach does.
> 
> Writing "./build" and "./install" should be fine as well.
> 

- You are right.

> Please give a rationale, why you choose to create an own directory for the
> library to install to.
> 

- since the library isn't intended to be used by any other program, it should go in a private dir.

> You can simplify the files section to:
> 
> %{_bindir}/*
> %{_libdir}/%{name}
> %{_mandir}/man1/*
> 
> I don't know if this package makes sense for EPEL. If you don't want to
> introduce it there, you can drop the buildroot, the clean section and the rm
> for buildroot in the install section.

- I thinking keeping the buildroot and clean is always a good idea, no harm in keeping these.

Comment 3 Hicham HAOUARI 2010-12-19 16:07:06 UTC
(In reply to comment #1)

> 
> You can simplify the files section to:
> 
> %{_bindir}/*
> %{_libdir}/%{name}
> %{_mandir}/man1/*
> 

I don't where the simplification is.

Comment 4 Hicham HAOUARI 2010-12-19 16:23:18 UTC
A second thought about Requiring SDL.

The program is basically overriding some calls in SDL, so I don't see why it should require it.

Comment 5 Volker Fröhlich 2010-12-19 16:45:16 UTC
Yes, keeping buildroot and clean doesn't harm.

Please add the rationale as a comment to the spec file. Maybe let upstream know.

The simplification is just a little more general syntax. It doesn't change the behaviour. No need to change it.

What would be the use to override something that isn't there?

Comment 6 Volker Fröhlich 2010-12-19 16:50:29 UTC
Or is it useful all on its own?

Comment 7 Hicham HAOUARI 2010-12-19 18:03:11 UTC
(In reply to comment #6)
> Or is it useful all on its own?

The application that we will be forced to minimize should be using SDL. So SDL is brought through the application.

The answer to your question is : sdlhack is not useful on its own, it becomes useful when you have an SDL fullscreen app that you want to force to minimize.

Comment 8 Kevin Kofler 2010-12-19 18:44:09 UTC
> SDLHack is a wrapper for SDL which lets you force full-screen games to
> minimize.

s/to minimize/into windowed mode/

"minimize" means to have the app shown only in the taskbar.

Comment 9 Kevin Kofler 2010-12-19 18:46:24 UTC
Hmmm, nevermind, actually the description is correct. (The readme introduces it first as "This is a simple LD_PRELOAD wrapper for SDL which enables switching away from a fullscreen game.", but if I read it more carefully, I see that it does this by minimizing entirely (which sucks, but that's not the package's problem).)

Comment 10 Volker Fröhlich 2010-12-19 19:51:27 UTC
OK, forget about the Requires.

Please add a comment, when you change your spec-file and also increment the release number and add an entry to the changelog. Otherwise the reviewer can't easily see, he's looking at an updated version.

Your current spec doesn't work.

Comment 11 Hicham HAOUARI 2010-12-19 20:44:51 UTC
(In reply to comment #10)
> OK, forget about the Requires.
> 
> Please add a comment, when you change your spec-file and also increment the
> release number and add an entry to the changelog. Otherwise the reviewer can't
> easily see, he's looking at an updated version.
> 
> Your current spec doesn't work.

What do you mean by "doesn't work" ? I test if it builds before submitting any changes

Comment 12 Volker Fröhlich 2010-12-19 21:04:30 UTC
+ install -Dpm 755 libsdlhack-i386.so libsdlhack-x86_64.so /home/makerpm/rpmbuild/BUILDROOT/sdlhack-1.1-1.fc13.x86_64/usr/lib64/sdlhack/libsdlhack.so
install: target `/home/makerpm/rpmbuild/BUILDROOT/sdlhack-1.1-1.fc13.x86_64/usr/lib64/sdlhack/libsdlhack.so' is not a directory
error: Bad exit status from /var/tmp/rpm-tmp.mfzHwm (%install)

Comment 13 Hicham HAOUARI 2010-12-19 22:23:09 UTC
(In reply to comment #12)
> + install -Dpm 755 libsdlhack-i386.so libsdlhack-x86_64.so
> /home/makerpm/rpmbuild/BUILDROOT/sdlhack-1.1-1.fc13.x86_64/usr/lib64/sdlhack/libsdlhack.so
> install: target
> `/home/makerpm/rpmbuild/BUILDROOT/sdlhack-1.1-1.fc13.x86_64/usr/lib64/sdlhack/libsdlhack.so'
> is not a directory
> error: Bad exit status from /var/tmp/rpm-tmp.mfzHwm (%install)

Fixed :

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


About not bumping release number, I don't think it is worth it since we are dealing with trivial issues.

Comment 14 Volker Fröhlich 2010-12-20 12:29:10 UTC
Package approved.

Rpmlint:
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

Source files match upstream:
sha256sum: 785104cb314447be4f3ab2fe13087cc1d6e6ca3bf171bbb57c85b099e977a534

BRs are listed.

Compiles and builds on at least x86_64 (EPEL6, F13, 14 and Rawhide) and ppc64 (EPEL6).

Package meets naming and versioning guidelines.

Specfile is properly named, is cleanly written and uses macros consistently.

License text included in package and open source-compatible;

Build root is correct.

Package doesn't own files or directories already owned by other packages.

%docs are not necessary for the proper functioning of the package. documentation is small, so no -docs subpackage is necessary.

No duplicates in %files.

Package contains code.

No shared libraries are added to the regular linker search paths.

Final provides and requires are sane:
/bin/bash
libc.so.6()(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.3.4)(64bit)
libdl.so.2()(64bit)
libdl.so.2(GLIBC_2.2.5)(64bit)
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)
rpmlib(PayloadIsXz) <= 5.2-1

Comment 15 Volker Fröhlich 2010-12-20 12:31:44 UTC
Forgot the provides:

libsdlhack.so((64bit)
sdlhack = 1.1-1.fc13
sdlhack(x86-64) = 1.1-1.fc13

Comment 16 Hicham HAOUARI 2010-12-20 12:41:53 UTC
thanks for reviewing this package

Comment 17 Hicham HAOUARI 2010-12-20 12:45:02 UTC
New Package SCM Request
=======================
Package Name: sdlhack
Short Description: Force full-screen games to minimize
Owners: hicham
Branches: f13 f14
InitialCC: hicham

Comment 18 Kevin Kofler 2010-12-20 13:06:42 UTC
> About not bumping release number, I don't think it is worth it since we are
> dealing with trivial issues.

It doesn't matter anymore for this particular package since it already passed review, but you're supposed to always bump Release and add a changelog entry for any change made after the initial review submission, no matter how trivial.

Comment 19 Kevin Fenzi 2010-12-21 06:17:28 UTC
Git done (by process-git-requests).

Comment 20 Fedora Update System 2010-12-21 23:10:45 UTC
sdlhack-1.1-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/sdlhack-1.1-1.fc14

Comment 21 Fedora Update System 2010-12-21 23:11:09 UTC
sdlhack-1.1-1.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/sdlhack-1.1-1.fc13

Comment 22 Fedora Update System 2010-12-22 19:50:29 UTC
sdlhack-1.1-1.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update sdlhack'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/sdlhack-1.1-1.fc13

Comment 23 Fedora Update System 2010-12-30 20:20:18 UTC
sdlhack-1.1-1.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 24 Fedora Update System 2010-12-30 20:21:59 UTC
sdlhack-1.1-1.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.


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