Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 194566 (915resolution)
Summary: | Review Request: 915resolution | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Chris Weyl <cweyl> |
Component: | Package Review | Assignee: | Paul F. Johnson <paul> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | matthias |
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-08-02 04:38:31 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Chris Weyl
2006-06-14 05:16:37 UTC
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. |