Bug 1052393 - Review Request: beignet - Open source implementation of the OpenCL for Intel GPUs
Summary: Review Request: beignet - Open source implementation of the OpenCL for Intel ...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Peter Lemenkov
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-01-13 18:34 UTC by Igor Gnatenko
Modified: 2014-01-30 03:36 UTC (History)
3 users (show)

Fixed In Version: beignet-0.3-9.48f8e5b.fc20
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-01-30 03:36:55 UTC
Type: ---
lemenkov: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Igor Gnatenko 2014-01-13 18:34:12 UTC
Spec URL: http://ignatenkobrain.fedorapeople.org/for-review/beignet.spec
SRPM URL: http://ignatenkobrain.fedorapeople.org/for-review/beignet-0.3-4.e427b3e.fc20.src.rpm
Description: Beignet is an open source implementaion of the OpenCL specification - a generic
compute oriented API. This code base contains the code to run OpenCL programs
on Intel GPUs which bsically defines and implements the OpenCL host functions
required to initialize the device, create the command queues, the kernels and
the programs and run them on the GPU.
Fedora Account System Username: ignatenkobrain

Comment 1 Christopher Meng 2014-01-14 05:46:59 UTC
Scratch: http://koji.fedoraproject.org/koji/taskinfo?taskID=6400516

-- Looking for mesa source code - not found, cl_khr_gl_sharing will be disabled.

Any explanation?

Comment 2 Igor Gnatenko 2014-01-14 07:10:21 UTC
(In reply to Christopher Meng from comment #1)
> Scratch: http://koji.fedoraproject.org/koji/taskinfo?taskID=6400516
> 
> -- Looking for mesa source code - not found, cl_khr_gl_sharing will be
> disabled.
> 
> Any explanation?
Need mesa-debuginfo for build, but when I'm adding it to BR - mock ignoring it.

Comment 3 Peter Lemenkov 2014-01-14 13:41:23 UTC
(In reply to Christopher Meng from comment #1)
> Scratch: http://koji.fedoraproject.org/koji/taskinfo?taskID=6400516
> 
> -- Looking for mesa source code - not found, cl_khr_gl_sharing will be
> disabled.
> 
> Any explanation?

Looks like it should be built with the Mesa sources. I think resolving this may be postponed (let's file a Bugzilla ticket later, right after the inclusion into Fedora).

Comment 4 Peter Lemenkov 2014-01-14 14:12:17 UTC
I'll review it.

Comment 5 Peter Lemenkov 2014-01-14 14:12:45 UTC
REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

- rpmlint is not silent

work ~/Desktop: rpmlint beignet-*
beignet.src: W: spelling-error %description -l en_US implementaion -> implementation, implementable, supplementation
beignet.src: W: spelling-error %description -l en_US bsically -> basically, classically

^^^ This time these are real issues. Please fix typos.

beignet.src:10: W: macro-in-comment %{version}
beignet.src:46: W: macro-in-comment %setup
beignet.src:46: W: macro-in-comment %{version}

^^^ I don't see these as a blockers. Just fyi - this can be silenced by escaping them with additional "%" sign (e.g. %%{version}, %%{setup}).

beignet.src:63: W: deprecated-grep [u'egrep']

^^^ Looks like false positive.

beignet.src:67: E: hardcoded-library-path in %{_prefix}/lib/beignet/

^^^ I'm afraid that's a blocker. This shold go into %{_libdir}/beignet/ . As for rpath it's ok since it falls into "Rpath for Internal Libraries" case:

https://fedoraproject.org/wiki/Packaging:Guidelines#Rpath_for_Internal_Libraries

beignet.src: W: invalid-url Source0: beignet-e427b3e.tar.gz

^^^ That's ok for SCM snapshots.

beignet.x86_64: W: spelling-error %description -l en_US implementaion -> implementation, implementable, supplementation
beignet.x86_64: W: spelling-error %description -l en_US bsically -> basically, classically

^^^ See my notes above.

beignet.x86_64: W: incoherent-version-in-changelog 0.3-3.e427b3e ['0.3-4.e427b3e.fc21', '0.3-4.e427b3e']

^^^ Please fix %changelog entry.

beignet.x86_64: W: non-conffile-in-etc /etc/OpenCL/vendors/intel-beignet.icd

^^^ That's intentional. This file shouldn't be edited by users.

beignet.x86_64: W: devel-file-in-non-devel-package /usr/lib/beignet/ocl_stdlib.h

^^^ I don't fully realize OpenCL development workflow, but maybe it's better to move these bits into the devel part?

I mostly concerned about these files:

/usr/lib/beignet/ocl_stdlib.h
/usr/lib/beignet/ocl_stdlib.h.pch

What's the purpose of /usr/lib/beignet/beignet.bc btw? Is this an OpenCL core or something like that?

beignet-devel.x86_64: W: spelling-error %description -l en_US implementaion -> implementation, implementable, supplementation

^^^ See my notes above.

4 packages and 0 specfiles checked; 1 errors, 13 warnings.
work ~/Desktop: 

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license (LGPLv2 or later)
+ The file, containing the text of the license(s) for the package, is included in %doc.

- The spec file must be written in American English w/o grammar errors. See my notes regarging rpmling issues above.

+ The spec file for the package is legible.
+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See koji link above.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files in some of the dynamic linker's default paths.
+ The package does NOT bundle copies of system libraries.
0 The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.

+/- Almost all header files are stored in a -devel package. See my note about the only C header which is stored in the main package.

0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package doesn't contain library files without a suffix (e.g. libfoo.so) in some of the dynamic linker's default paths.
+ The -devel package requires the base package using a fully versioned dependency: Requires: %{name}%{?_isa} = %{version}-%{release}
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ All filenames in rpm packages are valid UTF-8.


Please fix/comment my notes and I'll finish this.

Comment 6 Igor Gnatenko 2014-01-14 18:08:32 UTC
(In reply to Peter Lemenkov from comment #5)
> REVIEW:
> 
> Legend: + = PASSED, - = FAILED, 0 = Not Applicable
> 
> - rpmlint is not silent
> 
> work ~/Desktop: rpmlint beignet-*
> beignet.src: W: spelling-error %description -l en_US implementaion ->
> implementation, implementable, supplementation
> beignet.src: W: spelling-error %description -l en_US bsically -> basically,
> classically
> 
> ^^^ This time these are real issues. Please fix typos.
Fixed.
> beignet.src:10: W: macro-in-comment %{version}
> beignet.src:46: W: macro-in-comment %setup
> beignet.src:46: W: macro-in-comment %{version}
> 
> ^^^ I don't see these as a blockers. Just fyi - this can be silenced by
> escaping them with additional "%" sign (e.g. %%{version}, %%{setup}).
Unlike for me.
> beignet.src:63: W: deprecated-grep [u'egrep']
> 
> ^^^ Looks like false positive.
Really weird.
> beignet.src:67: E: hardcoded-library-path in %{_prefix}/lib/beignet/
> 
> ^^^ I'm afraid that's a blocker. This shold go into %{_libdir}/beignet/ . As
> for rpath it's ok since it falls into "Rpath for Internal Libraries" case:
> 
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Rpath_for_Internal_Libraries
I've tried to change buildsystem to use libdir, but package isn't useful.
Anyway we can't onetime use x86_64 and i386 packages. But submitted question to upstream.
> beignet.src: W: invalid-url Source0: beignet-e427b3e.tar.gz
> 
> ^^^ That's ok for SCM snapshots.
> 
> beignet.x86_64: W: spelling-error %description -l en_US implementaion ->
> implementation, implementable, supplementation
> beignet.x86_64: W: spelling-error %description -l en_US bsically ->
> basically, classically
> 
> ^^^ See my notes above.
> 
> beignet.x86_64: W: incoherent-version-in-changelog 0.3-3.e427b3e
> ['0.3-4.e427b3e.fc21', '0.3-4.e427b3e']
> 
> ^^^ Please fix %changelog entry.
Fixed.
> beignet.x86_64: W: non-conffile-in-etc /etc/OpenCL/vendors/intel-beignet.icd
> 
> ^^^ That's intentional. This file shouldn't be edited by users.
> 
> beignet.x86_64: W: devel-file-in-non-devel-package
> /usr/lib/beignet/ocl_stdlib.h
> 
> ^^^ I don't fully realize OpenCL development workflow, but maybe it's better
> to move these bits into the devel part?
No. w/o/ this files it doesn't work
> I mostly concerned about these files:
> 
> /usr/lib/beignet/ocl_stdlib.h
> /usr/lib/beignet/ocl_stdlib.h.pch
> 
> What's the purpose of /usr/lib/beignet/beignet.bc btw? Is this an OpenCL
> core or something like that?
yes.
> beignet-devel.x86_64: W: spelling-error %description -l en_US implementaion
> -> implementation, implementable, supplementation
> 
> ^^^ See my notes above.
> 
> 4 packages and 0 specfiles checked; 1 errors, 13 warnings.
> work ~/Desktop: 
> 
> + The package is named according to the  Package Naming Guidelines.
> + The spec file name matches the base package %{name}, in the format
> %{name}.spec.
> + The package meets the Packaging Guidelines.
> + The package is licensed with a Fedora approved license and meets the
> Licensing Guidelines.
> + The License field in the package spec file matches the actual license
> (LGPLv2 or later)
> + The file, containing the text of the license(s) for the package, is
> included in %doc.
> 
> - The spec file must be written in American English w/o grammar errors. See
> my notes regarging rpmling issues above.
> 
> + The spec file for the package is legible.
> + The package successfully compiles and builds into binary rpms on at least
> one primary architecture. See koji link above.
> + All build dependencies are listed in BuildRequires.
> 0 No need to handle locales.
> 0 No shared library files in some of the dynamic linker's default paths.
> + The package does NOT bundle copies of system libraries.
> 0 The package is not designed to be relocatable.
> + The package owns all directories that it creates.
> + The package does not list a file more than once in the spec file's %files
> listings.
> + Permissions on files are set properly.
> + The package consistently uses macros.
> + The package contains code, or permissible content.
> 0 No extremely large documentation files.
> + Anything, the package includes as %doc, does not affect the runtime of the
> application.
> 
> +/- Almost all header files are stored in a -devel package. See my note
> about the only C header which is stored in the main package.
> 
> 0 No static libraries.
> 0 No pkgconfig(.pc) files.
> 0 The package doesn't contain library files without a suffix (e.g.
> libfoo.so) in some of the dynamic linker's default paths.
> + The -devel package requires the base package using a fully versioned
> dependency: Requires: %{name}%{?_isa} = %{version}-%{release}
> + The package does NOT contain any .la libtool archives.
> 0 Not a GUI application.
> + The package does not own files or directories already owned by other
> packages.
> + All filenames in rpm packages are valid UTF-8.
> 
> 
> Please fix/comment my notes and I'll finish this.

New SPEC: http://ignatenkobrain.fedorapeople.org/for-review/beignet.spec
New SRPM: http://ignatenkobrain.fedorapeople.org/for-review/beignet-0.3-5.e427b3e.fc20.src.rpm

Comment 9 Peter Lemenkov 2014-01-16 10:40:46 UTC
(In reply to Igor Gnatenko from comment #8)
> Well.
> 
> All my patches in upstream[0][1][2], so drop it!
> 
> New SPEC: http://ignatenkobrain.fedorapeople.org/for-review/beignet.spec
> New SRPM:
> http://ignatenkobrain.fedorapeople.org/for-review/beignet-0.3-8.48f8e5b.fc20.
> src.rpm
> 
> Peter, please do review some faster =)
> 
> --
> [0]http://cgit.freedesktop.org/beignet/commit/
> ?id=9a94d1fb4db2b7bf98103b6ce7e162363041c878
> [1]http://cgit.freedesktop.org/beignet/commit/
> ?id=b63cd9bca17ef948f39454e642f375d1c3b8ca49
> [2]http://cgit.freedesktop.org/beignet/commit/
> ?id=48f8e5b1d27436c771175f7d407125c41f00171f

Ok, looks good now. I don't see any other issues so this package is


APPROVED.

Comment 10 Igor Gnatenko 2014-01-16 10:47:38 UTC
New Package SCM Request
=======================
Package Name: beignet
Short Description: Open source implementation of the OpenCL for Intel GPUs
Owners: ignatenkobrain
Branches: f20
InitialCC:

Comment 11 Gwyn Ciesla 2014-01-16 12:50:04 UTC
Git done (by process-git-requests).

Comment 12 Fedora Update System 2014-01-16 17:56:19 UTC
beignet-0.3-8.48f8e5b.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/beignet-0.3-8.48f8e5b.fc20

Comment 13 Fedora Update System 2014-01-18 04:26:17 UTC
beignet-0.3-8.48f8e5b.fc20 has been pushed to the Fedora 20 testing repository.

Comment 14 Fedora Update System 2014-01-21 05:51:06 UTC
beignet-0.3-9.48f8e5b.fc20 has been pushed to the Fedora 20 testing repository.

Comment 15 Fedora Update System 2014-01-30 03:36:55 UTC
beignet-0.3-9.48f8e5b.fc20 has been pushed to the Fedora 20 stable repository.


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