Bug 2036105

Summary:	Review Request: spirv-llvm8.0-translator - LLVM 8.0 to SPIRV Translator
Product:	[Fedora] Fedora	Reporter:	František Zatloukal <fzatlouk>
Component:	Package Review	Assignee:	Neal Gompa <ngompa13>
Status:	CLOSED RAWHIDE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	unspecified	Docs Contact:
Priority:	unspecified
Version:	rawhide	CC:	code, jun.miao, ngompa13, package-review
Target Milestone:	---	Flags:	ngompa13: fedora-review+
Target Release:	---
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:	spirv-llvm8.0-translator-8-1.20211223gita44863e.fc36	Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2022-01-20 23:30:08 UTC	Type:	Bug
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:	2036104
Bug Blocks:	2036109

Description František Zatloukal 2021-12-29 18:06:23 UTC

SPEC: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-driver-free/fedora-rawhide-x86_64/03083730-spirv-llvm8.0-translator/spirv-llvm8.0-translator.spec

SRPM: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-driver-free/fedora-rawhide-x86_64/03083730-spirv-llvm8.0-translator/spirv-llvm8.0-translator-8.0.0-1.fc36.src.rpm

Comment 1 František Zatloukal 2021-12-29 18:09:39 UTC

FYI: Just found out I'll need to add date/shortcommit into the nvr as it's a git snapshot really.

Comment 2 František Zatloukal 2022-01-10 19:31:49 UTC

SPEC: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-driver-free/fedora-rawhide-x86_64/03140537-spirv-llvm8.0-translator/spirv-llvm8.0-translator.spec

SRPM: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-driver-free/fedora-rawhide-x86_64/03140537-spirv-llvm8.0-translator/spirv-llvm8.0-translator-8-1.20211223gita44863e.fc36.src.rpm

Comment 3 Jun.Miao 2022-01-11 05:26:33 UTC

> Source0:        https://github.com/KhronosGroup/SPIRV-LLVM-Translator/archive/%{commit}/%{upstream_name}-%{shortcommit}.tar.gz

Please use proper SourceURL form as proscribed in the Guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags
In this case, that'd be "%{url}/archive/%{commit}/%{upstream_name}-%{shortcommit}.tar.gz"

Comment 4 František Zatloukal 2022-01-19 13:31:47 UTC

SPEC: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-driver-free/fedora-rawhide-x86_64/03198536-spirv-llvm8.0-translator/spirv-llvm8.0-translator.spec

SRPM: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-driver-free/fedora-rawhide-x86_64/03198536-spirv-llvm8.0-translator/spirv-llvm8.0-translator-8-1.20211223gita44863e.fc36.src.rpm

Comment 5 František Zatloukal 2022-01-19 13:32:43 UTC

(In reply to Jun.Miao from comment #3)
> > Source0:        https://github.com/KhronosGroup/SPIRV-LLVM-Translator/archive/%{commit}/%{upstream_name}-%{shortcommit}.tar.gz
> 
> Please use proper SourceURL form as proscribed in the Guidelines:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
> #_git_tags
> In this case, that'd be
> "%{url}/archive/%{commit}/%{upstream_name}-%{shortcommit}.tar.gz"

Addressed, added license into the package.

Comment 6 Neal Gompa 2022-01-20 13:25:12 UTC

Taking this review.

Comment 7 Neal Gompa 2022-01-20 13:26:10 UTC

>        -DLLVM_DIR=/usr/lib64/llvm8.0/lib/cmake/llvm \


This should use %{_libdir} rather than hardcoded /usr/lib64.

Comment 8 František Zatloukal 2022-01-20 14:20:51 UTC

SPEC: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-driver-free/fedora-rawhide-x86_64/03204358-spirv-llvm8.0-translator/spirv-llvm8.0-translator.spec

SRPM: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-driver-free/fedora-rawhide-x86_64/03204358-spirv-llvm8.0-translator/spirv-llvm8.0-translator-8-1.20211223gita44863e.fc36.src.rpm

Comment 9 Neal Gompa 2022-01-20 14:58:51 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 10 Gwyn Ciesla 2022-01-20 16:07:03 UTC

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/spirv-llvm8.0-translator

Comment 11 František Zatloukal 2022-01-20 23:30:08 UTC

Built in rawhide: spirv-llvm8.0-translator-8-1.20211223gita44863e.fc36

Comment 12 Dominik 'Rathann' Mierzejewski 2022-01-26 08:23:26 UTC

@fzatlouk please don't use globs for SONAMEs, you might miss SONAME bumps:

https://src.fedoraproject.org/rpms/spirv-llvm8.0-translator/blob/rawhide/f/spirv-llvm8.0-translator.spec#_74
%{_libdir}/libLLVMSPIRVLib.so.*

	
%files devel
%dir %{_includedir}/LLVMSPIRVLib/
%{_includedir}/LLVMSPIRVLib/

The %dir line is redundant.

Comment 13 František Zatloukal 2022-01-26 17:46:24 UTC

Thanks, should be addressed in spirv-llvm8.0-translator-8-2.20211223gita44863e.fc36 . The soname bump is technically impossible here, since it's just equal to llvm8.0 major, which will always be 8. Changed it though, there is no harm in doing that.