https://bugzilla.redhat.com/show_bug.cgi?id=1364618 Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@xxxxxxxxxxxxxxxxx |zbyszek@xxxxxxxxx Flags| |fedora-review? --- Comment #7 from Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> --- You should merge the embree-lib subpackage with the main package. The main package currently contains only a few small doc files. The .so file should be packaged in the -devel subpackage [https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages] Also: embree.src:25: W: mixed-use-of-spaces-and-tabs (spaces: line 25, tab: line 1) About the compiler flags: the guidelines say that distribution compilation flags should be used during compilation. How to achieve that is not fully specified, since it depends on the build system and usually there's a few different way to achieve the desired result. In this case the distro flags _are_ used: $ rpmbuild --eval %optflags -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic from build.log: /usr/bin/c++ -DTASKING_TBB -D__TARGET_AVX2__ -D__TARGET_AVX__ -I/builddir/build/BUILD/embree-2.10.0/include -I/builddir/build/BUILD/embree-2.10.0/x86_64-redhat-linux-gnu -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -fPIC -std=c++11 -fno-strict-aliasing -Wno-narrowing -fvisibility-inlines-hidden -fvisibility=hidden -DNDEBUG -O3 -o CMakeFiles/sys.dir/alloc.cpp.o -c /builddir/build/BUILD/embree-2.10.0/common/sys/alloc.cpp So the only issue is that this package requires AVX extensions, which means that it will not run on processors older then ~2011. Our guidelines specify that earlier CPUs should be supported, but in this case I'd say that this is OK: this program makes use and requires this hardware capability. Compiling a crippled version that will run slowly on all hardware isn't useful. What remains to be checked: - that we actually got it to compile properly, i.e. that it in fact uses AVX and runs fast - that it doesn't crash too horribly on unsupported CPUs I think you should build the tests and run them in %check. Without any tests it's hard to check that the package is compiled OK. The problem is that some of the examples don't compile, and some others fail. So I think you should compile: benchmark, verify, maybe some others?, and run them in %check. If the compilation host does not support necessary flags, the tests could be skipped: grep -q '\bavx\b' /proc/cpuinfo || { echo "CPU does not support AVX; skipping tests"; exit 0; } I'm not exactly sure what flags are required, this would probably require some experimentation. Upstream has an -examples subpackage, I think you could add one too, and put the tests binaries in it, in %{_libexecdir}/embree/. This way users can easily verify that the library works on their systems. You compile without ispc. But ispc is free software, and could be packaged for Fedora. Sounds like a useful things to have. I can do the review if you do the package ;) -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://lists.fedoraproject.org/admin/lists/package-review@xxxxxxxxxxxxxxxxxxxxxxx