Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: sdlhack - Force full-screen games to minimize
https://bugzilla.redhat.com/show_bug.cgi?id=657577
Summary: Review Request: sdlhack - Force full-screen games to minimize Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: hicham.haouari@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
Volker Fröhlich volker27@gmx.at changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |volker27@gmx.at AssignedTo|nobody@fedoraproject.org |volker27@gmx.at
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #1 from Volker Fröhlich volker27@gmx.at 2010-12-19 09:37:39 EST --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
Volker Fröhlich volker27@gmx.at changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #2 from Hicham HAOUARI hicham.haouari@gmail.com 2010-12-19 11:05:17 EST --- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #3 from Hicham HAOUARI hicham.haouari@gmail.com 2010-12-19 11:07:06 EST --- (In reply to comment #1)
You can simplify the files section to:
%{_bindir}/* %{_libdir}/%{name} %{_mandir}/man1/*
I don't where the simplification is.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #4 from Hicham HAOUARI hicham.haouari@gmail.com 2010-12-19 11:23:18 EST --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #5 from Volker Fröhlich volker27@gmx.at 2010-12-19 11:45:16 EST --- 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #6 from Volker Fröhlich volker27@gmx.at 2010-12-19 11:50:29 EST --- Or is it useful all on its own?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #7 from Hicham HAOUARI hicham.haouari@gmail.com 2010-12-19 13:03:11 EST --- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
Kevin Kofler kevin@tigcc.ticalc.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |kevin@tigcc.ticalc.org
--- Comment #8 from Kevin Kofler kevin@tigcc.ticalc.org 2010-12-19 13:44:09 EST ---
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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #9 from Kevin Kofler kevin@tigcc.ticalc.org 2010-12-19 13:46:24 EST --- 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).)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #10 from Volker Fröhlich volker27@gmx.at 2010-12-19 14:51:27 EST --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #11 from Hicham HAOUARI hicham.haouari@gmail.com 2010-12-19 15:44:51 EST --- (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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #12 from Volker Fröhlich volker27@gmx.at 2010-12-19 16:04:30 EST --- + 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)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #13 from Hicham HAOUARI hicham.haouari@gmail.com 2010-12-19 17:23:09 EST --- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
Volker Fröhlich volker27@gmx.at changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #14 from Volker Fröhlich volker27@gmx.at 2010-12-20 07:29:10 EST --- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #15 from Volker Fröhlich volker27@gmx.at 2010-12-20 07:31:44 EST --- Forgot the provides:
libsdlhack.so((64bit) sdlhack = 1.1-1.fc13 sdlhack(x86-64) = 1.1-1.fc13
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #16 from Hicham HAOUARI hicham.haouari@gmail.com 2010-12-20 07:41:53 EST --- thanks for reviewing this package
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #17 from Hicham HAOUARI hicham.haouari@gmail.com 2010-12-20 07:45:02 EST --- New Package SCM Request ======================= Package Name: sdlhack Short Description: Force full-screen games to minimize Owners: hicham Branches: f13 f14 InitialCC: hicham
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
Hicham HAOUARI hicham.haouari@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #18 from Kevin Kofler kevin@tigcc.ticalc.org 2010-12-20 08:06:42 EST ---
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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #19 from Kevin Fenzi kevin@tummy.com 2010-12-21 01:17:28 EST --- Git done (by process-git-requests).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |MODIFIED
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #20 from Fedora Update System updates@fedoraproject.org 2010-12-21 18:10:45 EST --- sdlhack-1.1-1.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/sdlhack-1.1-1.fc14
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #21 from Fedora Update System updates@fedoraproject.org 2010-12-21 18:11:09 EST --- sdlhack-1.1-1.fc13 has been submitted as an update for Fedora 13. https://admin.fedoraproject.org/updates/sdlhack-1.1-1.fc13
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #22 from Fedora Update System updates@fedoraproject.org 2010-12-22 14:50:29 EST --- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #23 from Fedora Update System updates@fedoraproject.org 2010-12-30 15:20:18 EST --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Fixed In Version| |sdlhack-1.1-1.fc13 Resolution| |ERRATA Last Closed| |2010-12-30 15:20:25
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
--- Comment #24 from Fedora Update System updates@fedoraproject.org 2010-12-30 15:21:59 EST --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=657577
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version|sdlhack-1.1-1.fc13 |sdlhack-1.1-1.fc14
package-review@lists.fedoraproject.org