Bug 2036099 - Review Request: intel-igc - Intel(R) Graphics Compiler for OpenCL(TM)
Summary: Review Request: intel-igc - Intel(R) Graphics Compiler for OpenCL(TM)
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Neal Gompa
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 2036098
Blocks: 1942132 2036135
TreeView+ depends on / blocked
 
Reported: 2021-12-29 17:54 UTC by František Zatloukal
Modified: 2022-02-01 15:43 UTC (History)
4 users (show)

Fixed In Version: intel-igc-1.0.9933-1.fc36
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2022-02-01 15:43:03 UTC
Type: Bug
Embargoed:
ngompa13: fedora-review+


Attachments (Terms of Use)

Comment 2 František Zatloukal 2022-01-18 22:03:39 UTC
Spec cleaned up, updated to the latest upstream release, fixed to build with gcc 12.

Comment 3 Fabio Valentini 2022-01-18 22:12:37 UTC
Sorry for the drive-by comment, but I saw this package just streaming by on the package-review list.
Note that you should never use (TM) or (R) in package summaries or descriptions:
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_trademarks_in_summary_or_description

Comment 5 František Zatloukal 2022-01-18 23:35:31 UTC
(In reply to Fabio Valentini from comment #3)
> Sorry for the drive-by comment, but I saw this package just streaming by on
> the package-review list.
> Note that you should never use (TM) or (R) in package summaries or
> descriptions:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/
> #_trademarks_in_summary_or_description

Thanks, (TM)/(R) and their unicode representations removed. Also renamed the specfile to match actual package name.

Comment 6 Dominik 'Rathann' Mierzejewski 2022-01-26 13:11:01 UTC
Source1: https://github.com/intel/vc-intrinsics/archive/%{vc_commit}/vc-intrinsics-%{vc_shortcommit}.tar.gz
Source2: https://github.com/KhronosGroup/SPIRV-Tools/archive/v%{spirv_tools_tag}/SPIRV-Tools-v%{spirv_tools_tag}.tar.gz
Source3: https://github.com/KhronosGroup/SPIRV-Headers/archive/%{spirv_headers_tag}/SPIRV-Headers-%{spirv_headers_tag}.tar.gz


%files
%{_libdir}/libiga64.so*
%{_libdir}/libigc.so*
%{_libdir}/libigdfcl.so*

This will miss any SONAME changes.

%{_libdir}/igc/NOTICES.txt

This looks like a %doc or %license candidate.

%files devel
%{_includedir}/igc/*
%{_includedir}/iga/*
%{_includedir}/visa/*

This makes the igc, iga and visa subdirectories in %{_includedir} unowned. Drop the "/*" part.

Comment 7 Dominik 'Rathann' Mierzejewski 2022-01-26 13:12:20 UTC
I hit submit too quickly.

(In reply to Dominik 'Rathann' Mierzejewski from comment #6):
> Source1: https://github.com/intel/vc-intrinsics/archive/%{vc_commit}/vc-intrinsics-%{vc_shortcommit}.tar.gz
> Source2: https://github.com/KhronosGroup/SPIRV-Tools/archive/v%{spirv_tools_tag}/SPIRV-Tools-v%{spirv_tools_tag}.tar.gz
> Source3: https://github.com/KhronosGroup/SPIRV-Headers/archive/%{spirv_headers_tag}/SPIRV-Headers-%{spirv_headers_tag}.tar.gz

These need either Provides: bundled(*) tags or be unbundled.

Comment 9 František Zatloukal 2022-01-28 21:42:40 UTC
All should hopefully be addressed.

For bundling, hopefully, the situation will improve in the future, there is upstream request already: https://github.com/intel/intel-graphics-compiler/issues/219

Comment 10 Neal Gompa 2022-01-29 19:42:30 UTC
Taking this review.

Comment 11 Neal Gompa 2022-01-29 19:44:51 UTC
> %license LICENSE.md
> %license %{_libdir}/igc/NOTICES.txt
> %{_libdir}/libiga64.so.1*
> %{_libdir}/libigc.so.1*
> %{_libdir}/libigdfcl.so.1*

This should be broken out to a libs subpackage and for the library sonames, you should use ".so.1{,.*}" instead of ".so.1*" to ensure you avoid some pitfalls around globbing.

The devel subpackage should depend on the libs subpackage, and the main package should depend on the libs subpackage.

Comment 13 František Zatloukal 2022-01-30 00:44:34 UTC
Everything should be resolved :)

Comment 14 Neal Gompa 2022-01-30 01:30:57 UTC
> Requires:      %{name} = %{version}-%{release}

This is not needed in the libs subpackage.

>     -DIGC_OPTION__LLVM_PREFERRED_VERSION='13.0.0' \

This should probably have the same trick you applied elsewhere to make sure the correct LLVM version is set there.

Comment 16 František Zatloukal 2022-01-30 10:56:04 UTC
(In reply to Neal Gompa from comment #14)
> > Requires:      %{name} = %{version}-%{release}
> 
> This is not needed in the libs subpackage.
> 
> >     -DIGC_OPTION__LLVM_PREFERRED_VERSION='13.0.0' \
> 
> This should probably have the same trick you applied elsewhere to make sure
> the correct LLVM version is set there.

Applied.

Comment 17 Neal Gompa 2022-01-30 18:43:08 UTC
Review notes:

* Packaging complies with the guidelines
* Package builds and installs
* No serious issues from rpmlint
* Licensing is correct and license files are correctly installed

PACKAGE APPROVED.

Comment 18 Gwyn Ciesla 2022-01-31 18:25:54 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/intel-igc

Comment 19 František Zatloukal 2022-02-01 15:43:03 UTC
Built in rawhide: intel-igc-1.0.9933-1.fc36


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