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.

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.

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)?

(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.

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.

(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

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.uk> 0.5-2
- fixed silly mistake in the spec file
- added ia64 and sparcx86 patches

* Sun 24 Dec 2006 Paul F. Johnson <paul.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.uk> 0.4-11

(you get the idea)

(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... :)

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.

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.

(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.

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

+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!

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.

Please branch for F-8.  Thanks! :)

cvs done.