Hide Forgot
Spec URL: https://nickc.fedorapeople.org/annobin.spec SRPM URL: https://nickc.fedorapeople.org/annobin-1.0-1.fc26.src.rpm Description: A plugin for GCC that records extra information in the files that it compiles. This information can be used to analyze the files, either by static tools that scan binaries for specific purposes, or by the dynamic linker when it loads the files. Note - the intent is that, assuming that this package is accepted, the redhat-rpm-macros package will be updated to require it, and to add an invocation of the plugin on the gcc command line. In that way all future compiled binaries will contain the information recorded by this plugin. The package includes a few example shell scripts that consume the information generated by the plugin. Of these hardended.sh is probably of the most interest. It checks compiled binaries to make sure that they have been built with all of the recommended hardening options. Fedora Account System Username: nickc
The SRPM link is a 403, so it cannot be reviewed. Please correct this.
Sorry - bad permissions on the file. Now fixed.
Apologies for the delay on this. Things got hectic. tl;dr: work needed (see Issues:) Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. Note: These BR are not needed: gcc gcc-c++ See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2 - Some content found to have MIT/X11 (BSD like) licenses. This needs to be in the License: field - Why are you explicitly excluding the LICENSE and COPYING3 files? Those need to be installed with the %license macro - This package does not own some of the directories it requires or alternately depend on a package that requires it. In all likelihood, you need to have this package `Requires: gcc` to ensure that its directory structure is properly owned. I also doubt anyone would want to install this plugin without also having gcc available. - /usr/lib/gcc/x86_64-redhat-linux/7/plugin is also owned by gcc-gdb-plugin. This is *not* a blocker, but it's worth asking the gcc folks if they should just have this owned by some common package if it's going to start being used by multiple plugins. - Drop the %clean section; it's not required on any supported version of Fedora or EPEL anymore ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files in private %_libdir subdirectory (see attachment). Verify they are not in ld path. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. 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]: 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. [!]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "MIT/X11 (BSD like)", "GPL (v2 or later)", "*No copyright* GPL (v3)", "Unknown or generated", "GPL (v3 or later)". 43 files have unknown license. Detailed output of licensecheck in /dev/shm/1451407-annobin/licensecheck.txt [!]: License file installed when any subpackage combination is installed. [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/lib/gcc/x86_64-redhat- linux/7, /usr/lib/gcc, /usr/lib/gcc/x86_64-redhat-linux [!]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /usr/lib/gcc/x86_64-redhat- linux/7/plugin(gcc-gdb-plugin) [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. [-]: 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. [!]: 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. [x]: Package is not known to require an ExcludeArch tag. [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]: Package requires other packages for directories it uses. [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 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]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [!]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) Note: %clean present but not required [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [!]: Final provides and requires are sane (see attachments). [x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in annobin- debuginfo [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [!]: %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]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Uses parallel make %{?_smp_mflags} macro. [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [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: annobin-1.0-1.fc27.x86_64.rpm annobin-debuginfo-1.0-1.fc27.x86_64.rpm annobin-1.0-1.fc27.src.rpm annobin.x86_64: W: incoherent-version-in-changelog 1.0-1.fc26 ['1.0-1.fc27', '1.0-1'] annobin.x86_64: W: no-documentation annobin.x86_64: W: hidden-file-or-dir /usr/lib/.build-id annobin.x86_64: W: hidden-file-or-dir /usr/lib/.build-id annobin.x86_64: W: no-manual-page-for-binary built-by.sh annobin.x86_64: W: no-manual-page-for-binary check-abi.sh annobin.x86_64: W: no-manual-page-for-binary hardened.sh annobin.src: W: strange-permission annobin-1.0.tar.xz 660 3 packages and 0 specfiles checked; 0 errors, 8 warnings. Rpmlint (debuginfo) ------------------- Checking: annobin-debuginfo-1.0-1.fc27.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- sh: /usr/bin/python: No such file or directory annobin.x86_64: W: incoherent-version-in-changelog 1.0-1.fc26 ['1.0-1.fc27', '1.0-1'] annobin.x86_64: W: no-documentation annobin.x86_64: W: hidden-file-or-dir /usr/lib/.build-id annobin.x86_64: W: hidden-file-or-dir /usr/lib/.build-id annobin.x86_64: W: no-manual-page-for-binary built-by.sh annobin.x86_64: W: no-manual-page-for-binary check-abi.sh annobin.x86_64: W: no-manual-page-for-binary hardened.sh 2 packages and 0 specfiles checked; 0 errors, 7 warnings. Requires -------- annobin (rpmlib, GLIBC filtered): /bin/bash libc.so.6()(64bit) libgcc_s.so.1()(64bit) libm.so.6()(64bit) libstdc++.so.6()(64bit) rtld(GNU_HASH) annobin-debuginfo (rpmlib, GLIBC filtered): Provides -------- annobin: annobin annobin(x86-64) annobin-debuginfo: annobin-debuginfo annobin-debuginfo(x86-64) Unversioned so-files -------------------- annobin: /usr/lib/gcc/x86_64-redhat-linux/7/plugin/annobin.so Source checksums ---------------- https://nickc.fedorapeople.org/annobin-1.0.tar.xz : CHECKSUM(SHA256) this package : b2e232a91158f9e949655375281fd091c360b3cfae51e561997d635dec3ac1ed CHECKSUM(SHA256) upstream package : b2e232a91158f9e949655375281fd091c360b3cfae51e561997d635dec3ac1ed Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02 Command line :/usr/bin/fedora-review -b 1451407 -m fedora-rawhide-x86_64 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api, C/C++ Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
Hi Stephen, > Apologies for the delay on this. Things got hectic. No worries - thanks very much for taking the time to run the review. > tl;dr: work needed (see Issues:) No surprises there - this is my first contribution. :-) Anyway, on to the review. I have created an updated version of the submissions which addresses most of the points raised: Spec URL: https://nickc.fedorapeople.org/annobin.spec SRPM URL: https://nickc.fedorapeople.org/annobin-2.0-1.fc26.src.rpm > Issues: > ======= > - All build dependencies are listed in BuildRequires, except for any that > are listed in the exceptions section of Packaging Guidelines. > Note: These BR are not needed: gcc gcc-c++ > See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2 OK, I will remove them. (Does it matter if they are present ?) (FYI: I know that the Fedora Review Guidelines webpage contains that link above, but the Exceptions_2 entry on the Guidelines page does not actually specify which requirements can be omitted from the BuildRequires tag). > - Some content found to have MIT/X11 (BSD like) licenses. This needs to be in > the License: field OK, this is now done. Note - the fedora-review tool flags quite a few files as "Unknown or generated". I assume that I can ignore these. Also it flags the LICENSE file as having "*No copyright* GPL (v3)". I assume that this is OK too. > - Why are you explicitly excluding the LICENSE and COPYING3 files? Those need > to be installed with the %license macro I was basing my annobin.spec file on the spec file used by the odb package. (This is another gcc plugin, so it seemed like a good idea at the time). That package excludes the LICENSE/COPYING3 files and does not have a %license entry, so I thought that that was the correct thing to do for plugins. Anyway I have now corrected this issue and added the %license entry. > - This package does not own some of the directories it requires or alternately > depend on a package that requires it. In all likelihood, you need to have this > package `Requires: gcc` to ensure that its directory structure is properly > owned. I also doubt anyone would want to install this plugin without also > having gcc available. True. > - /usr/lib/gcc/x86_64-redhat-linux/7/plugin is also owned by gcc-gdb-plugin. > This is *not* a blocker, but it's worth asking the gcc folks if they should > just have this owned by some common package if it's going to start being used > by multiple plugins. Oh how I wish... :-) Does Fedora support meta-packages ? Ie could we have a gcc-plugin meta package that owns this directory, and maybe a documentation directory as well. It could also link somehow to each of the currently existing gcc plugin packages. In a related note, I have been trying to get the gcc folks to take more responsibility for plugins, and to start treating them as an official part of the compiler. But so far I have been blocked each time I try. Plugins are seen as being a bit of a mess and not suitable for inclusion in a real compiler project. :-( > - Drop the %clean section; it's not required on any supported version of Fedora > or EPEL anymore Done. > [!]: Final provides and requires are sane (see attachments). Umm, not sure what needs to be done here. Could you provide me with a pointer or two please ? > [!]: %check is present and all tests pass. I have not done this (yet) as I am still trying to work out a clean way to test the plugin. I hope however that this is not a showstopper. > annobin.x86_64: W: no-documentation This should be done. > annobin.x86_64: W: no-manual-page-for-binary built-by.sh > annobin.x86_64: W: no-manual-page-for-binary check-abi.sh > annobin.x86_64: W: no-manual-page-for-binary hardened.sh These are shell scripts. As such I have tried to make them self-documenting. Is there a way to tell rpmlint that this is the situation ? Cheers Nick
(In reply to Nick Clifton from comment #4) ... > > Issues: > > ======= > > - All build dependencies are listed in BuildRequires, except for any that > > are listed in the exceptions section of Packaging Guidelines. > > Note: These BR are not needed: gcc gcc-c++ > > See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2 > > OK, I will remove them. (Does it matter if they are present ?) > They're not harmful. Actually I meant not to include that in the final list of issues, but forgot to delete it. > (FYI: I know that the Fedora Review Guidelines webpage contains that link > above, > but the Exceptions_2 entry on the Guidelines page does not actually specify > which > requirements can be omitted from the BuildRequires tag). > Yeah, actually in retrospect it's probably better to include them, on the off-chance they ever got removed from the default buildroot. > > > - Some content found to have MIT/X11 (BSD like) licenses. This needs to be in > > the License: field > > OK, this is now done. > > Note - the fedora-review tool flags quite a few files as "Unknown or > generated". > I assume that I can ignore these. Also it flags the LICENSE file as having > "*No copyright* GPL (v3)". I assume that this is OK too. > Don't assume that the "Unknown or generated" ones are ignorable. Best to double-check. In general, we assume that if they have no license information, they're covered by the project license. > > > - Why are you explicitly excluding the LICENSE and COPYING3 files? Those need > > to be installed with the %license macro > > I was basing my annobin.spec file on the spec file used by the odb package. > (This is another gcc plugin, so it seemed like a good idea at the time). > That > package excludes the LICENSE/COPYING3 files and does not have a %license > entry, > so I thought that that was the correct thing to do for plugins. > > Anyway I have now corrected this issue and added the %license entry. > Thanks > > > - This package does not own some of the directories it requires or alternately > > depend on a package that requires it. In all likelihood, you need to have this > > package `Requires: gcc` to ensure that its directory structure is properly > > owned. I also doubt anyone would want to install this plugin without also > > having gcc available. > > True. > > > - /usr/lib/gcc/x86_64-redhat-linux/7/plugin is also owned by gcc-gdb-plugin. > > This is *not* a blocker, but it's worth asking the gcc folks if they should > > just have this owned by some common package if it's going to start being used > > by multiple plugins. > > Oh how I wish... :-) Does Fedora support meta-packages ? Ie could we have a > gcc-plugin meta package that owns this directory, and maybe a documentation > directory as well. It could also link somehow to each of the currently > existing > gcc plugin packages. As I said, this isn't a blocker, but it's completely acceptable to have a subpackage like `foo-filesystem` that contains only empty directory structures and have packages depend on that. It's recommended that anything that supports plugins should have either the main package or a -filesystem package provide the necessary framework. > > In a related note, I have been trying to get the gcc folks to take more > responsibility for plugins, and to start treating them as an official part > of the compiler. But so far I have been blocked each time I try. Plugins > are > seen as being a bit of a mess and not suitable for inclusion in a real > compiler > project. :-( > Above my pay-grade. > > > - Drop the %clean section; it's not required on any supported version of Fedora > > or EPEL anymore > > Done. > > > > [!]: Final provides and requires are sane (see attachments). > > Umm, not sure what needs to be done here. Could you provide me with a > pointer or two please ? > Sorry, I should have mentioned that this was just fallout from not having `Requires: gcc` > > > [!]: %check is present and all tests pass. > > I have not done this (yet) as I am still trying to work out a clean > way to test the plugin. I hope however that this is not a showstopper. > This is non-blocking. It's a SHOULD criterion. I marked it as [!] because %check wasn't present. > > > annobin.x86_64: W: no-documentation > > This should be done. > > > annobin.x86_64: W: no-manual-page-for-binary built-by.sh > > annobin.x86_64: W: no-manual-page-for-binary check-abi.sh > > annobin.x86_64: W: no-manual-page-for-binary hardened.sh > > These are shell scripts. As such I have tried to make them > self-documenting. Is there a way to tell rpmlint that this is the > situation ? > They're warnings, not errors. It's mostly a reminder in case you forgot to package the manual.
OK, review time: The `License:` field is not in the required format. See https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#License:_field It should be: ``` License: GPLv3+ and MIT ``` When you have both GPLv2+ and GPLv3+ code together, GPLv3+ covers both. The MIT/X11 license is represented by "MIT" in Fedora. Are `built-by.sh`, `check-abi.sh` and `hardened.sh` really tools that belong in /usr/bin, or are they internal to the package?
Hi Stephan, Thanks for the second review. I have uploaded a revised annobin.spec file with the License tag corrected as you indicated. The scripts (built-by, check-abi and hardended) are real tools that are intended to be run by users. (Well users of the annobin-plugin package, which in practice will only be developers and distributors). What they actually do is process the information that the annobin plugin inserts into compiled binaries. So the built-by script for example extract the information about the compiler used, whilst the hardended script checks that all of the appropriate hardening options have been enabled, and so on. Finally all of the "unknown/generated" results in the licensecheck.txt file are suitable for being covered by the project license. Cheers Nick
(In reply to Nick Clifton from comment #7) > Hi Stephan, > > Thanks for the second review. I have uploaded a revised annobin.spec file > with the License tag corrected as you indicated. > > The scripts (built-by, check-abi and hardended) are real tools that are > intended to be run by users. (Well users of the annobin-plugin package, > which in practice will only be developers and distributors). What they > actually do is process the information that the annobin plugin inserts into > compiled binaries. So the built-by script for example extract the > information about the compiler used, whilst the hardended script checks that > all of the appropriate hardening options have been enabled, and so on. > > Finally all of the "unknown/generated" results in the licensecheck.txt > file are suitable for being covered by the project license. > https://nickc.fedorapeople.org/annobin.spec still shows the wrong License: field.
(In reply to Stephen Gallagher from comment #8) > https://nickc.fedorapeople.org/annobin.spec still shows the wrong License: > field. Doh! Sorry about that. This should now be fixed. Cheers Nick
Package is approved.
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/annobin
Package built