Bug 194566 - (915resolution) Review Request: 915resolution
Review Request: 915resolution
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul F. Johnson
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-06-14 01:16 EDT by Chris Weyl
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-02 00:38:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Chris Weyl 2006-06-14 01:16:37 EDT
Spec URL: http://home.comcast.net/~ckweyl/915resolution.spec
SRPM URL: http://home.comcast.net/~ckweyl/915resolution-0.5.2-0.fc5.src.rpm
Description:
915resolution is a tool to modify the video BIOS of the 800 and 900 series
Intel graphics chipsets. This includes the 845G, 855G, and 865G chipsets, as
well as 915G, 915GM, and 945G chipsets. This modification is necessary to
allow the display of certain graphics resolutions for an Xorg or XFree86
graphics server.

915resolution's modifications of the BIOS are transient. There is no risk of
permanent modification of the BIOS. This also means that 915resolution must be
run every time the computer boots inorder for it's changes to take effect.

915resolution is derived from the tool 855resolution. However, the code
differs substantially. 915resolution's code base is much simpler.
915resolution also allows the modification of bits per pixel.
Comment 1 Thorsten Leemhuis 2006-06-14 01:34:39 EDT
Did you see Bug 186391 ? 

Side note: I don't agree with the conclusions in that Bug; people want it fixed
now and maybe for FC4, too -- so I'm fine with having 915resolution in Extras
(and I even once considered packaging it myself). But it probably should not be
in FE6 if the fixed/improved driver will be in FC6.
Comment 2 Chris Weyl 2006-06-14 14:32:26 EDT
No, I actually hadn't seen that.

I'm all for there being an updated xorg driver that does the same thing on a
more "seamless" basis, but until then, this is the only way many people can get
their screen resolutions working correctly.  (Like on this brand-spanking new
Dell Inspiron B130 sitting next to me <grin>)  This is a tactical, not strategic
approach, as Management would say;)

I'm ok with maintaining this for FC-4, FC-5 and obsoleting it as soon as the new
xorg driver is out.  If it saves one person the couple hours of "huh?" it caused
me it's worth it, IMHO.
Comment 3 Paul F. Johnson 2006-07-27 18:40:05 EDT
Let's make a start

"# will be set to -1 on build/release
Release:        0%{?dist}"

The release should reflect the release (in other words, the build number). It
goes up, not down

%description ends in a .

I can't see how this package would be instantated by the host machine. Is it
supposed to be called directly, as a service or needing integration into
xorg.conf (or similar)?
Comment 4 Chris Weyl 2006-07-27 18:51:40 EDT
(In reply to comment #3)
> Let's make a start
> 
> "# will be set to -1 on build/release
> Release:        0%{?dist}"
> 
> The release should reflect the release (in other words, the build number). It
> goes up, not down

Sorry, that could have been more clearly expressed...  It'll go to one.

> %description ends in a .

Isn't this only %summary which shouldn't end in a period?
 
> I can't see how this package would be instantated by the host machine. Is it
> supposed to be called directly, as a service or needing integration into
> xorg.conf (or similar)?

Right now, it's up to the user to configure.  I call it with the correct
resolution values at boot in /etc/rc.local, for instance.  It would certainly be
possible to wrap the entire thing in a service, but 1) that would still require
user configuration as to which video modes to override, and 2) it's not worth
that effort given the xorg driver has been in a state of not needing this "real
soon now" for the last several months.
Comment 5 Paul F. Johnson 2006-07-27 18:55:04 EDT
The release should already be one as soon as you package the original version.

Not sure about the user to configure bit.

Personally, I'd wrap it in a service and install a set of "default" modes (they
can all be commented out). At least everything is in place when the xorg driver
gets out of the stable and into the big, bad world.
Comment 6 Chris Weyl 2006-07-27 19:13:27 EDT
(In reply to comment #5)
> The release should already be one as soon as you package the original version.

Bumped.

> Not sure about the user to configure bit.
> 
> Personally, I'd wrap it in a service and install a set of "default" modes (they
> can all be commented out). At least everything is in place when the xorg driver
> gets out of the stable and into the big, bad world.

For a temporary (alebit turning into a long-term) fix, I still don't think it's
worth that effort -- as soon as the xorg update is released in core, this
program's function is obviated.  If someone is installing this package, they're
going to need to know what to do with it regardless of if there's a service
wrapper or not.

I did, however, stick a README.fedora in this release, with a brief "here's how
to get it going on boot" synopsis...

Spec URL: http://home.comcast.net/~ckweyl/915resolution.spec
SRPM URL: http://home.comcast.net/~ckweyl/915resolution-0.5.2-1.fc5.src.rpm
Comment 7 Paul F. Johnson 2006-07-27 19:31:30 EDT
Is the .fc5 at the end of the src.rpm your own rename? if it is, please don't.
src rpms are release agnostic.

The spec file still has the release as 0?{%dist}. This should be 2 as it's the
2nd release made. Remove the 0/-1 comment, it's wrong!

You don't need the make clean (unless the source tarball has a pre-made
configure with pre-made bits and pieces) in %build

I'd prefer install -m 755 for the package into %{_sbindir}. Also, as it's only
one program going into %{_sbindir}, give it the name in %files

The changelog hasn't altered. Each time you change something, it gets
documented, so it should read

%changelog
* Mon 25 Dec 2006 Paul F. Johnson <paul@all-the-johnsons.co.uk> 0.5-2
- fixed silly mistake in the spec file
- added ia64 and sparcx86 patches

* Sun 24 Dec 2006 Paul F. Johnson <paul@all-the-johnsons.co.uk> 0.5-1
- added BR foo
- removed R bar
- added a b c and d into docs
- chmod 0755 sbindir-foobar
- bumped to new version

* Sun 17 Dec 2006 Paul F. Johnson <paul@all-the-johnsons.co.uk> 0.4-11

(you get the idea)
Comment 8 Chris Weyl 2006-07-27 19:45:07 EDT
(In reply to comment #7)
> Is the .fc5 at the end of the src.rpm your own rename? if it is, please don't.
> src rpms are release agnostic.

I define %dist in my ~/.rpmmacros file, for my own sanity when building
packages...  It's not a rename.

> The spec file still has the release as 0?{%dist}. This should be 2 as it's the
> 2nd release made. Remove the 0/-1 comment, it's wrong!

Hit reload on your browser, it's out there at 1 :)

> You don't need the make clean (unless the source tarball has a pre-made
> configure with pre-made bits and pieces) in %build

It does :)

> I'd prefer install -m 755 for the package into %{_sbindir}. Also, as it's only
> one program going into %{_sbindir}, give it the name in %files

Are these personal preferences, or do you consider them blockers?

> The changelog hasn't altered. Each time you change something, it gets
> documented, so it should read
[...snip...]
> (you get the idea)

Again, hit reload... :)
Comment 9 Paul F. Johnson 2006-07-27 19:50:59 EDT
As it stands, I've not built it yet (will do on the laptop - I can then give it
a try). However, I would consider the install -m 0755 to be essential. The file
going into %{_sbindir}, it makes life easier for us mere reviewers (plus, it's
only one file, so makes things clear to others wanting to package)

Hitting reload worked, though the release should be 2.
Comment 10 Paul F. Johnson 2006-07-28 05:29:57 EDT
Good
----

rpmlint is clean for the binary, debuginfo and src rpms
requires list is fine
permissions correctly set
builds cleanly in mock (x86)
README.fedora added to the package
tarball version matches upstream

Bad
---

It really does need to be enclosed in a wrapper with an example script for use
in %{_sysconfdir} - as it stands, it's one of those applications that you
install and wonder why you did - at least with something in %{_sysconfdir} users
will know what to do (or where to look!)

Minor
-----

release version is one off - this one should be 2 (there is no release 0!)
"public domain" as a license is fine for FE, but has caused issues in Germany.
This is not just FC, but lots of others. As it stands, FC is fine with it as a
license.

Fix the issue in BAD and I'm happy to approve it.
Comment 11 Chris Weyl 2006-08-01 18:39:06 EDT
(In reply to comment #10)
> Bad
> ---
> 
> It really does need to be enclosed in a wrapper with an example script for use
> in %{_sysconfdir} - as it stands, it's one of those applications that you
> install and wonder why you did - at least with something in %{_sysconfdir} users
> will know what to do (or where to look!)
>
> Fix the issue in BAD and I'm happy to approve it.

Is hdparm a service?  They both perform the same sort of function -- run once at
boot to tweak various interface settings of a physical device.

If someone has installed this package, then either they know what they're doing
(and can find README.fedora), or they don't and this package shouldn't try to do
anything with their system.  If they do, they're still going to have to
configure it based on the particulars of their hardware, so the amount of
user-work is the same either way.  The package-supplied README is quite detailed
in the particulars of the proper use of the program.

A full-blown sysv-style service wrapper around would turn this very simple
package into a much more complex beast, with addtional scripts, documentation,
additional bits in the spec, etc, than it warrants.  It introduces additional
complexity at a net of questionable benefit -- especially when we consider that
this package, while useful to those who need its functionality _now_, is
predestined to have a very limited lifespan.
Comment 12 Paul F. Johnson 2006-08-01 19:05:40 EDT
Okay.

The considered opinion of others is that as it stands, it would be simple enough
to create a script, but it isn't needed.

On that basis...

APPROVED

Don't forget to close this bug and set the resolve bug to NEXTRELEASE
Comment 13 Chris Weyl 2006-08-02 00:38:31 EDT
+Import to CVS
+Add to owners.list
+Bump release, build for devel
+devel build succeeds
+Request branching (FC-4, FC-5)
+Close bug

Thanks for the review!
Comment 14 Matthias Saou 2006-08-02 08:06:22 EDT
I'm a bit disapointed to see this package has gone in, when apparently many
people were opposed to it in bug #186391. I also agree with Paul about the
package lacking user-friendliness, which is why I have included an init script
and a /etc/sysconfig/ file in my 855resolution package.

Please consider merging stuff from my 855resolution package into this one.
Comment 15 Chris Weyl 2007-10-14 20:51:25 EDT
Please branch for F-8.  Thanks! :)
Comment 16 Kevin Fenzi 2007-10-14 21:24:33 EDT
cvs done.

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