Spec URL: https://sagitter.fedorapeople.org/nativejit/nativejit.spec SRPM URL: https://sagitter.fedorapeople.org/nativejit/nativejit-0.0-1.fc33.src.rpm Description: NativeJIT will be required by next release of COPASI. Fedora Account System Username: sagitter
Why such an odd URL? If this is truly an external dependency of COPASI, shouldn't you be packaging the upstream sources? Those appear to be at https://github.com/BitFunnel/NativeJIT
COPASI team uses own mirrors for COPASI dependencies (https://github.com/copasi/copasi-dependencies); since it is code recently modified/adapted (instead BitFunnel/NativeJIT commits code in April 2018 last time), i chose to take away 'copasi-dependencies/tree/master/src/NativeJIT' in a new separated GitLab project (https://gitlab.com/anto.trande/nativejit) and push there all my changes instead of filling of patches the SRPM.
It seems like upstream hasn’t really had either commits nor bug reports since late 2018, so I could understand the potential need for a “maintained fork.” Is that what’s happening here? It seems reasonable as long as you plan to continue maintaining it and as long as you do not diverge from the original upstream in ways that would make the fork unsuitable for dependent packages other than COPASI. I think a comment in the spec file describing these plans would be very helpful. The package fails to build in mock for me: + /usr/bin/cmake -S . -B x86_64-redhat-linux-gnu -DCMAKE_C_FLAGS_RELEASE:STRING=-DNDEBUG -DCMAKE_CXX_FLAGS_RELEASE:STRING=-DNDEBUG -DCMAKE_Fortran_FLAGS_RELEASE:STRING=-DNDEBUG -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON -DCMAKE_INSTALL_DO_STRIP:BOOL=OFF -DCMAKE_INSTALL_PREFIX:PATH=/usr -DINCLUDE_INSTALL_DIR:PATH=/usr/include -DLIB_INSTALL_DIR:PATH=/usr/lib64 -DSYSCONF_INSTALL_DIR:PATH=/etc -DSHARE_INSTALL_PREFIX:PATH=/usr/share -DLIB_SUFFIX=64 -DBUILD_SHARED_LIBS:BOOL=ON -DCMAKE_BUILD_TYPE:STRING=Release '-DCMAKE_CXX_FLAGS_RELEASE:STRING=-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -DNDEBUG' '-DCMAKE_C_FLAGS_RELEASE:STRING=-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -DNDEBUG' -DCMAKE_VERBOSE_MAKEFILE:BOOL=TRUE -DCMAKE_SKIP_INSTALL_RPATH:BOOL=YES -DNATIVEJIT_VERSION_MAJOR=0 -DNATIVEJIT_VERSION=0 CMake Error at CMakeLists.txt:3 (cmake_minimum_required): cmake_minimum_required called with unknown argument "[FATAL_ERROR]". -- The CXX compiler identification is GNU 11.0.0 -- The C compiler identification is GNU 11.0.0 -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Check for working CXX compiler: /usr/bin/g++ - skipped -- Detecting CXX compile features -- Detecting CXX compile features - done -- Detecting C compiler ABI info -- Detecting C compiler ABI info - done -- Check for working C compiler: /usr/bin/gcc - skipped -- Detecting C compile features -- Detecting C compile features - done -- ######################################## NativeJIT is root -- Found GTest: /usr/lib64/libgtest.so -- Configuring incomplete, errors occurred! See also "/builddir/build/BUILD/nativejit-v0.0/x86_64-redhat-linux-gnu/CMakeFiles/CMakeOutput.log". See also "/builddir/build/BUILD/nativejit-v0.0/x86_64-redhat-linux-gnu/CMakeFiles/CMakeError.log". I am attaching CMakeOutput.log and CMakeError.log. In addition to the problem with cmake_minimum_required, it looks like there are problems with using C++ flags on a C source, which are being treated as errors.
Created attachment 1760237 [details] CMakeOutput.log Fails to build with mock
Created attachment 1760239 [details] CMakeError.log Fails to build with mock
> The package fails to build in mock for me: I fixed the source archive. Spec URL: https://sagitter.fedorapeople.org/nativejit/nativejit.spec SRPM URL: https://sagitter.fedorapeople.org/nativejit/nativejit-0.0-1.fc33.src.rpm > It seems reasonable as long as you plan to continue maintaining it and as long as you do not diverge from the original upstream in ways that would make the fork unsuitable for dependent packages other than COPASI. I think a comment in the spec file describing these plans would be very helpful. I agree, added comments to the SPEC file.
The library compiles with -mSSE4.2 so that the implementations of the two overloads of NativeJIT::BitOp::GetNonZeroBitCount can use POPCNT/LZCNT. You are already ExclusiveArch: x86_64, but the base x86_64 architecture does not include these instructions. I recently raised this issue (admittedly, based on a misunderstanding of the library in question at the time) and got FESCo to record a policy: https://pagure.io/packaging-committee/issue/1044. So you seem to have a few options: 1. Proceed with packaging this library as-is, with SSE4.2 instructions possibly sprinkled throughout due to the -mSSE4.2 option. This is allowable since it is a library. In the COPASI application, you will have to provide runtime feature detection and a fallback that completely avoids calling any functions from this library, since any of them could include SSE4 instructions. 2. Patch this library to add runtime CPU detection and fallback implementations, compiling the optimized versions of these two routines with __attribute__((__target__("sse4.2"))) or in a separate translation unit with -mSSE4.2. Remove -mSSE4.2 from the overall compiler flags. It may be difficult to avoid paying a possibly-significant performance penalty for added indirection at the granularity of a tiny inlinable function. 3. Give up the speed advantage of POPCNT/LZCNT: remove -mSSE4.2 from the overall compiler flags, and replace the two overloads of NativeJIT::BitOp::GetNonZeroBitCount unconditionally with a generic algorithm like https://graphics.stanford.edu/~seander/bithacks.html#CountBitsSetParallel. This has the advantage of simplicity, and of not adding indirection. The performance penalty will probably vary from trivial to substantial depending on how heavily the JIT’ed routines use these operations. What do you think?
I guess a fourth allowable option is for the COPASI application to cleanly exit when SSE4.2 is not supported.
Thank you Benjamin for your in-depth analysis but i have absolutely not idea of what's the best choice. I compile this software as-is provided by upstream, observations like yours are beyond my knowledge.
Okay, I looked into it further, and it seems COPASI already implements option “1.” Look at https://github.com/copasi/COPASI/blob/0dddbe0d84e4248270ff65d06ea2b3997cc4f3b9/copasi/math/CJitCompiler.cpp to see how all JIT is disabled when a runtime CPU feature test shows SSE4 is not available. So from a policy perspective, everything is fine. You do need a quick feature test in the spec file to avoid running the tests on build hosts without SSE4.2 (like mine!). Your %check could look like this: if grep -E '\bsse4_2\b' /proc/cpuinfo >/dev/null then %ctest -- -VV else echo 'No SSE4.2 support on build host; skipping tests' 1>&2 fi Or, if it’s more to your liking, use the other package you have under review: BuildRequires: google-cpu_features BuildRequires: jq if list_cpu_features -j | jq -e '.flags | index("sse4_2")' >/dev/null then %ctest -- -VV else echo 'No SSE4.2 support on build host; skipping tests' 1>&2 fi Personally, I would favor also adding a comment near the ExcludeArch mentioning the SSE4.2 requirement and linking https://pagure.io/packaging-committee/issue/1044, and perhaps mentioning the SSE4.2 requirement in the package description as well. ----- Based on https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files, you should write the shared library globs as %{_libdir}/libCodeGen.so.0 %{_libdir}/libNativeJIT.so.0 instead of %{_libdir}/libCodeGen.so.* %{_libdir}/libNativeJIT.so.* ----- I’m not a big fan of claiming something as vague and generic as /usr/include/Temporary, but I’m not sure there’s a reasonable way to avoid it. ----- I’m going to go ahead and claim this for review. Could you post an updated version that skips the tests when necessary and adjusts the shared library globs, and I’ll review that? Thanks.
Ok, a couple of other preliminary things from rpmlint: nativejit.x86_64: E: summary-too-long C Library for high-performance just-in-time compilation of expressions involving C data structures That’s easy to fix. And this: nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::rip nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::RegisterBase::c_names nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::rbp nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::RegisterBase::c_sizes nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::rsp nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::FunctionBuffer::EndFunctionBodyGeneration(NativeJIT::FunctionSpecification const&) nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::X64CodeGenerator::CodePrinter::PrintBytes(unsigned int, unsigned int) nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::CodeBuffer::AllocateLabel() nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::X64CodeGenerator::CodePrinter::CodePrinter(NativeJIT::X64CodeGenerator&) nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::X64CodeGenerator::IsDiagnosticsStreamAvailable() const nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::FunctionBuffer::BeginFunctionBodyGeneration() nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::LogThrowImpl(char const*, char const*, unsigned int, char const*, char const*, ...) nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::IosMiniStateRestorer::~IosMiniStateRestorer() nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::CodeBuffer::Emit32(unsigned int) nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::IosMiniStateRestorer::IosMiniStateRestorer(std::basic_ios<char, std::char_traits<char> >&) nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::X64CodeGenerator::GetDiagnosticsStream() const nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::Label::Label() nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::X64CodeGenerator::OpCodeName(NativeJIT::OpCode) nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::CodeBuffer::Emit8(unsigned char) nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::FunctionBuffer::GetEntryPoint() const nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::X64CodeGenerator::CodePrinter::GetPointerName(unsigned int) nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::CodeBuffer::CurrentPosition() const nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 NativeJIT::FunctionSpecification::FunctionSpecification(Allocators::IAllocator&, int, unsigned int, unsigned int, unsigned int, NativeJIT::FunctionSpecification::BaseRegisterType, std::basic_ostream<char, std::char_traits<char> >*) I haven’t had time to try to understand why that is happening.
(In reply to code from comment #10) > Okay, I looked into it further, and it seems COPASI already implements > option “1.” Look at > https://github.com/copasi/COPASI/blob/ > 0dddbe0d84e4248270ff65d06ea2b3997cc4f3b9/copasi/math/CJitCompiler.cpp to see > how all JIT is disabled when a runtime CPU feature test shows SSE4 is not > available. So from a policy perspective, everything is fine. > > You do need a quick feature test in the spec file to avoid running the tests > on build hosts without SSE4.2 (like mine!). Your %check could look like this: > > if grep -E '\bsse4_2\b' /proc/cpuinfo >/dev/null > then > %ctest -- -VV > else > echo 'No SSE4.2 support on build host; skipping tests' 1>&2 > fi > > Or, if it’s more to your liking, use the other package you have under review: > > BuildRequires: google-cpu_features > BuildRequires: jq > > if list_cpu_features -j | jq -e '.flags | index("sse4_2")' >/dev/null > then > %ctest -- -VV > else > echo 'No SSE4.2 support on build host; skipping tests' 1>&2 > fi > > Personally, I would favor also adding a comment near the ExcludeArch > mentioning the SSE4.2 requirement and linking > https://pagure.io/packaging-committee/issue/1044, and perhaps mentioning the > SSE4.2 requirement in the package description as well. > > ----- > > Based on > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_listing_shared_library_files, you should write the shared library globs as > > %{_libdir}/libCodeGen.so.0 > %{_libdir}/libNativeJIT.so.0 > > instead of > > %{_libdir}/libCodeGen.so.* > %{_libdir}/libNativeJIT.so.* > > ----- > > I’m not a big fan of claiming something as vague and generic as > /usr/include/Temporary, but I’m not sure there’s a reasonable way to avoid > it. > > ----- > > I’m going to go ahead and claim this for review. Could you post an updated > version that skips the tests when necessary and adjusts the shared library > globs, and I’ll review that? Thanks. Spec URL: https://sagitter.fedorapeople.org/nativejit/nativejit.spec SRPM URL: https://sagitter.fedorapeople.org/nativejit/nativejit-0.0-2.fc33.src.rpm Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=63078250 (In reply to code from comment #11) > > And this: > > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::rip > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::RegisterBase::c_names > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::rbp > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::RegisterBase::c_sizes > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::rsp > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::FunctionBuffer::EndFunctionBodyGeneration(NativeJIT:: > FunctionSpecification const&) > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::X64CodeGenerator::CodePrinter::PrintBytes(unsigned int, unsigned > int) > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::CodeBuffer::AllocateLabel() > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::X64CodeGenerator::CodePrinter::CodePrinter(NativeJIT:: > X64CodeGenerator&) > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::X64CodeGenerator::IsDiagnosticsStreamAvailable() const > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::FunctionBuffer::BeginFunctionBodyGeneration() > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::LogThrowImpl(char const*, char const*, unsigned int, char const*, > char const*, ...) > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::IosMiniStateRestorer::~IosMiniStateRestorer() > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::CodeBuffer::Emit32(unsigned int) > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::IosMiniStateRestorer::IosMiniStateRestorer(std::basic_ios<char, > std::char_traits<char> >&) > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::X64CodeGenerator::GetDiagnosticsStream() const > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::Label::Label() > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::X64CodeGenerator::OpCodeName(NativeJIT::OpCode) > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::CodeBuffer::Emit8(unsigned char) > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::FunctionBuffer::GetEntryPoint() const > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::X64CodeGenerator::CodePrinter::GetPointerName(unsigned int) > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::CodeBuffer::CurrentPosition() const > nativejit.x86_64: W: undefined-non-weak-symbol /usr/lib64/libNativeJIT.so.0 > NativeJIT::FunctionSpecification::FunctionSpecification(Allocators:: > IAllocator&, int, unsigned int, unsigned int, unsigned int, > NativeJIT::FunctionSpecification::BaseRegisterType, std::basic_ostream<char, > std::char_traits<char> >*) > > I haven’t had time to try to understand why that is happening. I already seen them and asked to upstream maintainer: https://github.com/copasi/copasi-dependencies/issues/7
The undefined symbols in /usr/lib64/libNativeJIT.so.0 are all defined in /usr/lib64/libCodeGen.so.0. So the problem is that the NativeJIT library fails to link the CodeGen library. Adding target_link_libraries(NativeJIT PUBLIC CodeGen) in src/NativeJIT/CMakeLists.txt seems to fix it. It may be that the dependency could be PRIVATE rather than PUBLIC; I do not understand the interaction of these CMake linking options with C++ well enough to be sure. ----- By the way, -DCMAKE_VERBOSE_MAKEFILE:BOOL=TRUE is (harmlessly) redundant with -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON in the expansion of %cmake.
Spec URL: https://sagitter.fedorapeople.org/nativejit/nativejit.spec SRPM URL: https://sagitter.fedorapeople.org/nativejit/nativejit-0.1-1.fc33.src.rpm Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=63154175
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== Issues ===== - The -devel subpackage needs to Require: cmake-filesystem for /usr/lib64/cmake ===== Other notes ===== - ExclusiveArch is present and properly justified. Once the package is approved you must file a bug in Bugzilla for each unsupported primary architecture, and blocking the relevant tracker bug. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_build_failures for details, and https://bugzilla.redhat.com/show_bug.cgi?id=1897661 for an example. ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: ldconfig not called in %post and %postun for Fedora 28 and later. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. [x]: Development (unversioned) .so files in -devel subpackage, if present. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "Expat License". 19 files have unknown license. Detailed output of licensecheck in /home/reviewer/1933988-nativejit/20210306/1933988-nativejit/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/lib64/cmake [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [-]: Package is not known to require an ExcludeArch tag. ExclusiveArch present and properly justified. Once the package is approved you must file a bug in Bugzilla for each unsupported primary architecture, and blocking the relevant tracker bug. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_build_failures for details, and https://bugzilla.redhat.com/show_bug.cgi?id=1897661 for an example. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 10240 bytes in 1 files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package requires other packages for directories it uses. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [x]: Sources can be downloaded from URI in Source: tag Note: Could not download Source0: https://gitlab.com/anto.trande/nativejit/-/archive/v0.1/nativejit-v0.1.tar.bz2 See: https://docs.fedoraproject.org/en-US/packaging- guidelines/SourceURL/ It seems like GitLab whitelists user agents and denies all others, and this affects fedora-review. In fact, the URI is fine and this is not a problem. [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: SourceX tarball generation or download is documented. Note: Package contains tarball without URL, check comments This diagnostic is incorrect; everything is fine. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Fully versioned dependency in subpackages if applicable. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: SourceX is a working URL. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: nativejit-0.1-1.fc35.x86_64.rpm nativejit-devel-0.1-1.fc35.x86_64.rpm nativejit-debuginfo-0.1-1.fc35.x86_64.rpm nativejit-debugsource-0.1-1.fc35.x86_64.rpm nativejit-0.1-1.fc35.src.rpm nativejit-devel.x86_64: W: no-documentation 5 packages and 0 specfiles checked; 0 errors, 1 warnings. Rpmlint (debuginfo) ------------------- Checking: nativejit-debuginfo-0.1-1.fc35.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- nativejit-devel.x86_64: W: no-documentation 4 packages and 0 specfiles checked; 0 errors, 1 warnings. Requires -------- nativejit (rpmlib, GLIBC filtered): libCodeGen.so.0()(64bit) libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3.1)(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(CXXABI_1.3.9)(64bit) rtld(GNU_HASH) nativejit-devel (rpmlib, GLIBC filtered): libCodeGen.so.0()(64bit) libNativeJIT.so.0()(64bit) nativejit(x86-64) nativejit-debuginfo (rpmlib, GLIBC filtered): nativejit-debugsource (rpmlib, GLIBC filtered): Provides -------- nativejit: NativeJIT libCodeGen.so.0()(64bit) libNativeJIT.so.0()(64bit) nativejit nativejit(x86-64) nativejit-devel: nativejit-devel nativejit-devel(x86-64) nativejit-debuginfo: debuginfo(build-id) nativejit-debuginfo nativejit-debuginfo(x86-64) nativejit-debugsource: nativejit-debugsource nativejit-debugsource(x86-64) Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10 Command line :/usr/bin/fedora-review -b 1933988 Buildroot used: fedora-rawhide-x86_64 Active plugins: Shell-api, C/C++, Generic Disabled plugins: Java, SugarActivity, Haskell, R, Perl, Ocaml, PHP, Python, fonts, Ruby Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
(In reply to code from comment #15) > Package Review > ============== > > Legend: > [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated > > ===== Issues ===== > > - The -devel subpackage needs to Require: cmake-filesystem for > /usr/lib64/cmake (Overwrited) Spec URL: https://sagitter.fedorapeople.org/nativejit/nativejit.spec SRPM URL: https://sagitter.fedorapeople.org/nativejit/nativejit-0.1-1.fc33.src.rpm > > ===== Other notes ===== > > - ExclusiveArch is present and properly justified. Once the package is > approved > you must file a bug in Bugzilla for each unsupported primary architecture, > and blocking the relevant tracker bug. See > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_architecture_build_failures > for details, and https://bugzilla.redhat.com/show_bug.cgi?id=1897661 for an > example. > Okay.
I’m not sure that requiring all of CMake from -devel to own the directory is the right approach. Is the goal to support EPEL7, where this package is not available? If so, it seems like it would be better to conditionalize it: %if 0%{?epel} == 7 Requires: cmake%{?_isa} %else Requires: cmake-filesystem %endif Actually, what I would do is %if 0%{?epel} != 7 Requires: cmake-filesystem %endif and then in %files devel, co-own the directory on EPEL7: %if 0%{?epel} == 7 %dir %{_libdir}/cmake/ %endif ---- That said, I’ll go ahead and approve the package as-is. Thanks for working through all of these details. ---- If you have the chance to review one of mine, would you mind doing https://bugzilla.redhat.com/show_bug.cgi?id=1936138? I need it for unbundling from grpc. It’s a header-only C library that shouldn’t take long to review. Thanks!
> Is the goal to support EPEL7, where this package is not available? (Here I was referring to cmake-filesystem.)
(In reply to code from comment #17) > I’m not sure that requiring all of CMake from -devel to own the directory is > the right approach. Is the goal to support EPEL7, where this package is not > available? If so, it seems like it would be better to conditionalize it: > > %if 0%{?epel} == 7 > Requires: cmake%{?_isa} > %else > Requires: cmake-filesystem > %endif > > Actually, what I would do is > > %if 0%{?epel} != 7 > Requires: cmake-filesystem > %endif > > and then in %files devel, co-own the directory on EPEL7: > > %if 0%{?epel} == 7 > %dir %{_libdir}/cmake/ > %endif > > ---- EPEL7 support is not expected for now. However, in Fedora, 'cmake' requires 'cmake-filesystem' (that wouldn't make sense without cmake): $ repoquery --requires cmake | grep filesystem Last metadata expiration check: 0:03:36 ago on lun 8 mar 2021, 19:09:01. cmake-filesystem(x86-32) = 3.18.3-1.fc33 cmake-filesystem(x86-64) = 3.18.3-1.fc33 cmake-filesystem(x86-64) = 3.18.4-2.fc33 > > That said, I’ll go ahead and approve the package as-is. Thanks for working > through all of these details. Thank you. > > ---- > > If you have the chance to review one of mine, would you mind doing > https://bugzilla.redhat.com/show_bug.cgi?id=1936138? I need it for > unbundling from grpc. It’s a header-only C library that shouldn’t take long > to review. Thanks! Assigned.
The idea of a -filesystem package is that, when all you need is the directory to store files in “in case they are needed,” you can depend on the -filesystem package instead of pulling in another system (in this case CMake) that might not be otherwise required. The first part of https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_and_directory_ownership and the subsection https://docs.fedoraproject.org/en-US/packaging-guidelines/#_the_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function both talk about these -filesystem packages. For example, suppose I have a package that requires nativejit-devel but does not use CMake. If nativejit-devel Requires cmake-filesystem, that is just one trivial extra package added to my buildroot. If nativejit-devel Requires cmake, that is 17 unnecessary packages added to my buildroot, totaling 24 M downloaded and 107 M installed, including guile22 and python3. This is not catastrophic, but you can see why people try to avoid it.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/nativejit
FEDORA-2021-6bb23e4603 has been submitted as an update to Fedora 34. https://bodhi.fedoraproject.org/updates/FEDORA-2021-6bb23e4603
FEDORA-2021-6bb23e4603 has been pushed to the Fedora 34 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-6bb23e4603 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-6bb23e4603 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2021-6bb23e4603 has been pushed to the Fedora 34 stable repository. If problem still persists, please make note of it in this bug report.