Bug 1031342

*** Bug 1031341 has been marked as a duplicate of this bug. ***

It's not ok to use git am

(In reply to Christopher Meng from comment #2)
> It's not ok to use git am

oh.. I just cargo-cult'd that over from Bastien's original .spec file.  There aren't actually any patches applied in the package, so I guess I can just delete that line?

(In reply to Rob Clark from comment #3)
> (In reply to Christopher Meng from comment #2)
> > It's not ok to use git am
> 
> oh.. I just cargo-cult'd that over from Bastien's original .spec file. 
> There aren't actually any patches applied in the package, so I guess I can
> just delete that line?

Yes, delete the entire block

==========
git init
if [ -z "$GIT_COMMITTER_NAME" ]; then
    git config user.email "xorg-x11-drv-freedreno-owner"
    git config user.name "Fedora xorg-x11-drv-freedreno maintainers"
fi
git add .
git commit -a -q -m "%{version} baseline."

git am -p1 %{patches} < /dev/null
==========

Looks like there were some patches some time, but they're gone.

ok.. removed that, and the "BuildRequires: git".. the .spec is still in same place:

http://people.freedesktop.org/~robclark/rpmbuild/xorg-x11-drv-freedreno/xorg-x11-drv-freedreno.spec

(and I bumped the release #.. although wasn't 100% sure if that is appropriate for a pre-release rpm.. it was at least convenient for me testing it)

new koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6190666

(In reply to Rob Clark from comment #5)
> (and I bumped the release #.. although wasn't 100% sure if that is
> appropriate for a pre-release rpm.. it was at least convenient for me
> testing it)

every published iteration of the spec needs a release bump, the other parties can much easier follow the changes, so you did it correct :-)

A couple comments on the spec :
- Remove the '%define version 1.0.0' line, use the Version: tag directly instead.
- Swap the Summary: and Name: lines.
- Source: tag should be Source0:.
- About the URL tag, isn't there a better homepage than http://www.x.org ?
- Remove the %defattr line in the %files section, it's obsolete.
- In the changelog, add the release after the version.

Apart from these minor issues, the spec looks clean and it should be ready for a formal review.

Fix comment 7 and:

autoreconf -f -i --> autoreconf -fiv

Please upload new ones once you've fixed all above, then I will run a formal review.

(In reply to Xavier Bachelot from comment #7)
> - About the URL tag, isn't there a better homepage than http://www.x.org ?

well, that is the same that xorg-x11-drv-intel.spec uses..

I could instead use http://freedreno.github.io/ (although I don't really want people to be confused and think that the github trees are the upstream trees) or http://cgit.freedesktop.org/xorg/driver/xf86-video-freedreno/

(In reply to Rob Clark from comment #9)
> (In reply to Xavier Bachelot from comment #7)
> > - About the URL tag, isn't there a better homepage than http://www.x.org ?
> 
> well, that is the same that xorg-x11-drv-intel.spec uses..
> 
Sure, the Intel driver could have a better URL: tag too ;-)
I would have expected something alike http://www.x.org/wiki/intel/ or www.x.org/wiki/radeon/, but indeed, as the page doesn't exist (yet) for freedreno, it would have to be created first. 

> I could instead use http://freedreno.github.io/ (although I don't really
> want people to be confused and think that the github trees are the upstream
> trees) or http://cgit.freedesktop.org/xorg/driver/xf86-video-freedreno/

Agreed, I don't think the git trees make good homepages.

In any case, that's mostly cosmetic and is not an issue for the review, but imho you should consider creating at least a placeholder in the X.org wiki when you have a few spare cycles.

(In reply to Xavier Bachelot from comment #7)
> - Swap the Summary: and Name: lines.

just double checking, are you sure about that?  What I have is at least consistent with xorg-x11-drv-intel and xorg-x11-drv-{intel,nouveau,omap}?

(not claiming to be an expert about this, but just trying to stay consistent with the other ddx drivers)

(In reply to Rob Clark from comment #11)
> (In reply to Xavier Bachelot from comment #7)
> > - Swap the Summary: and Name: lines.
> 
> just double checking, are you sure about that?  What I have is at least
> consistent with xorg-x11-drv-intel and xorg-x11-drv-{intel,nouveau,omap}?
> 
> (not claiming to be an expert about this, but just trying to stay consistent
> with the other ddx drivers)

This is cosmetic only, not a blocker at all, I just felt it was more logical to have Name: before Summary:. And the other X driver specs are old, so they might not be the state of the art. The canonical form for the spec can be seen in /etc/rpmdevtools/spectemplate-minimal.spec but it seems even that is outdated, as there is still BuildRoot:, a %clean section, etc...

Sorry for bringing confusion with my comments. To be clearer, the Name: and Summary: as well as the URL: changes are cosmetic, do as you see fit. The others needs to be fixed, but I let Christopher point out the real issues in the formal review.

(In reply to Xavier Bachelot from comment #12)
> (In reply to Rob Clark from comment #11)
> > (In reply to Xavier Bachelot from comment #7)
> > > - Swap the Summary: and Name: lines.
> > 
> > just double checking, are you sure about that?  What I have is at least
> > consistent with xorg-x11-drv-intel and xorg-x11-drv-{intel,nouveau,omap}?
> > 
> > (not claiming to be an expert about this, but just trying to stay consistent
> > with the other ddx drivers)
> 
> This is cosmetic only, not a blocker at all, I just felt it was more logical
> to have Name: before Summary:. And the other X driver specs are old, so they
> might not be the state of the art. The canonical form for the spec can be
> seen in /etc/rpmdevtools/spectemplate-minimal.spec but it seems even that is
> outdated, as there is still BuildRoot:, a %clean section, etc...
> 
> Sorry for bringing confusion with my comments. To be clearer, the Name: and
> Summary: as well as the URL: changes are cosmetic, do as you see fit. The
> others needs to be fixed, but I let Christopher point out the real issues in
> the formal review.

ok, thanks for the explaination, it makes more sense now.  Somehow I was thinking you meant swap the name/summary *values* which didn't make any sense to me (ie, "Summary: xorg-x11-drv-freedreno"..)

Hi, I've uploaded another version, which I think addresses all the review comments:

Spec URL: http://people.freedesktop.org/~robclark/rpmbuild/xorg-x11-drv-freedreno/xorg-x11-drv-freedreno.spec
SRPM URL: http://people.freedesktop.org/~robclark/rpmbuild/SRPMS/xorg-x11-drv-freedreno-1.0.0-3.fc20.src.rpm

koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6198606

note that I left the URL as-is for now, but I have a ticket open with the freedesktop.org admins to create wiki page.. I can update the .spec later whenever they eventually get around to that.

Thanks everyone for putting up with the rookie questions/mistakes :-)

1. Drop BuildRoot tag.

2. Suggestion:

find $RPM_BUILD_ROOT -regex ".*\.la$" | xargs rm -f --

-->

find %{buildroot} -name '*.la' -delete

3. Add a dot after X.org X11 Qualcomm Adreno video driver in %desciption:

X.org X11 Qualcomm Adreno video driver.

----------

Review is not avilable because in my location:

DEBUG util.py:281:  Error: No Package found for pkgconfig(libdrm_freedreno)

@christopher, pkgconfig(libdrm_freedreno) is only available on ARM, so if you don't have an ARM machine to run fedora-review, you'll have to manually run the sanity checks using packages from a koji scratch build.
Are you still up for the review if you don't have ARM hardware at hand ?

@rob, rather than pointing at a git repo, I would suggest using :
Source0: http://xorg.freedesktop.org/archive/individual/driver/xf86-video-freedreno-%{version}.tar.bz2
Again, this is cosmetic.

Oops... 

I have many arm boards in my hand, but none of them are officially supported by Fedora(or said that kernel Linux).

So I can't perform the review. Although I can say this package is fine, it's improper and irresponsible to continue. 

Quitting.

Rob, I guess you are probably already in touch with the Fedora ARM buddies, and hopefully they would be able to help with this review. May be you could drop a mail to arm.org ?

I know the perfect reviewer for this package. Hans, I hope you don't mind me dropping some (more) work on your shoulders ;-)

Hi,

(In reply to Xavier Bachelot from comment #19)
> I know the perfect reviewer for this package. Hans, I hope you don't mind me
> dropping some (more) work on your shoulders ;-)

Ok, I'll bite, I'm now yum distro-syncing my trimslice to F-20, when that is done I'll review this (likely tomorrow).

Regards,

Hans

I can confirm that the usabily test has passed for this ddx driver with current fedora userspace. But the test device (ifc6410) still miss device tree hence fedora kernel support.

I still wonder if the driver could be auto-selected for a given soc.
Like what is hardcoded from the kernel in pci cases:
http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/common/xf86pciBus.c

I'm going to test the latest patch (xa) with mesa 10.2 backported to f20.

(In reply to Nicolas Chauvet (kwizart) from comment #21)
> I can confirm that the usabily test has passed for this ddx driver with
> current fedora userspace. But the test device (ifc6410) still miss device
> tree hence fedora kernel support.

just fwiw, the strategy I am taking is to just try to get all the userspace stuff in place upstream for now, and maintain non-fedora kernels.  It is much easier for end users with (for example) an ifc6410 board to take a vanilla fedora userspace and only a custom kernel / boot.img, vs if we ask them to also recompile mesa, ddx, etc.

But I'm hopeful that the upstream situation is improving for snapdragon.. the pace of patches upstream has picked up, and qcom now a linaro member, etc.

> I still wonder if the driver could be auto-selected for a given soc.
> Like what is hardcoded from the kernel in pci cases:
> http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/common/xf86pciBus.c

fwiw, Thierry Reding had a recent patchset to support autoloading for non-pci devices, which looks like the right way to go long term.

http://lists.freedesktop.org/archives/xorg-devel/2014-February/040671.html

We could either cherry pick those back into fedora xserver, or take the lazy way for the short term and ask users to manually copy over an xorg .conf file.

> I'm going to test the latest patch (xa) with mesa 10.2 backported to f20.

btw, I found out about COPR the other day.. I'm sorta thinking it would be useful to have COPR that is something equivalent to xorg-edgers which could pick up latest mesa, ddx, etc.

Hi Rob,

Sorry for being very very slow with reviewing this. Here is a full review:

Good:
=====
- rpmlint checks return:
 xorg-x11-drv-freedreno.armv7hl: W: no-documentation
 3 packages and 0 specfiles checked; 0 errors, 1 warnings.
 This can safely be ignored
- package meets naming guidelines
- package meets packaging guidelines
- license (MIT) OK, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on F-19 arm
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

Needs work:
==========
-Drop the BuildRoot tag, it is not longer used
-Switch Source0 to: http://ftp.x.org/pub/individual/driver/xf86-video-freedreno-1.0.0.tar.bz2
 rather then using a git snapshot
-Drop the autoreconf call, it is not needed when using a tarbal which has been properly made with "make dist"
-Drop the "BuildRequires: autoconf automake libtool" they are only needed for autoreconf
-Replace "make V=1" with  "make %{?_smp_mflags} V=1"
-Replace "make install DESTDIR=$RPM_BUILD_ROOT PREFIX=''" with "%make_install" this does the same, except that it drops the nonsense PREFIX='', which is not needed

If you can re-sping the package fixing the needs work items, then I'll approve it and we can mvoe forward with including it in Fedora proper.

Regards,

Hans

updated package the spec is written from scratch its based on the xorg-x11-drv-omap driver already in Fedora
SPEC:http://ausil.us/packages/xorg-x11-drv-freedreno.spec
SRPM: http://ausil.us/packages/xorg-x11-drv-freedreno-1.0.0-1.20140503.fc21.src.rpm

(In reply to Dennis Gilmore from comment #24)
> updated package the spec is written from scratch its based on the
> xorg-x11-drv-omap driver already in Fedora
> SPEC:http://ausil.us/packages/xorg-x11-drv-freedreno.spec
> SRPM:
> http://ausil.us/packages/xorg-x11-drv-freedreno-1.0.0-1.20140503.fc21.src.rpm

Thanks for the new version, looks good, APRROVED!

*** Bug 1093952 has been marked as a duplicate of this bug. ***

Package Name: xorg-x11-drv-freedreno
Short Description: xorg ddx driver for snapdragon/adreno arm SoC's
Owners: ausil rclark
Branches: f20
InitialCC:

Package Name: xorg-x11-drv-freedreno
Short Description: xorg ddx driver for snapdragon/adreno arm SoC's
Owners: ausil rclark
Branches: f20
InitialCC:

New Package SCM Request
=======================
Package Name: xorg-x11-drv-freedreno
Short Description: xorg ddx driver for snapdragon/adreno arm SoC's
Owners: ausil rclark
Branches: f20
InitialCC:

Git done (by process-git-requests).

package is now available in rawhide