[Bug 1350884] Review Request: mspgcc - Rebase of GCC for the MSP430 to TI / Red Hat upstream

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1350884



--- Comment #32 from Brandon Nielsen <nielsenb@xxxxxxxxxxx> ---
Spec URL:
https://copr-be.cloud.fedoraproject.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01577708-msp430-elf-toolchain/msp430-elf-toolchain.spec
SRPM URL:
https://copr-be.cloud.fedoraproject.org/results/nielsenb/msp430-development-tools/srpm-builds/01577708/msp430-elf-toolchain-9.2.0.0-3.fc32.src.rpm

(In reply to Andy Mender from comment #30)
> After some fiddling, I managed to run `fedora-review` from the COPR build,
> thanks!
> 
> > BuildRequires:	gmp-devel
> > BuildRequires:	libmpc-devel
> > %if 0%{?fedora} >= 32
> > BuildRequires:	pkgconfig(mpfr)
> > %else
> > BuildRequires:	mpfr-devel
> > %endif
> > BuildRequires:	pkgconfig(ncurses)
> > BuildRequires:	sed
> > BuildRequires:	texinfo
> > BuildRequires:	pkgconfig(zlib)
> 
> I would check whether it's possible to replace the "*-devel" lines with
> "pkgconfig(foo)" like you did in the other cases.
> 

I don't see a pkgconfig provided by gmp-devel or libmpc-devel.

> > cd binutils
> > %make_install
> > cd -
> 
> > cd gcc
> > PATH=$PWD/../bin:$PATH
> > %make_install
> > # Reset the path
> > PATH=%{base_path}
> > cd -
> 
> > cd gdb
> > %make_install
> > cd -
> 
> Add the "-p" flag to %make_install to preserve timestamps.
> 

Done.

> Quite a bit of clean-up needs to be done still, sorry :(. There is a load of
> header files, libtools and static objects which shouldn't be there. The full
> review matrix is below (I included the full review + rpmlint in attached
> file):
> 

I got rid of the remaining libtool archive. I'm afraid I don't understand what
header files or static objects shouldn't be there. This is a compiler, those
are required for it to function.

> Package Review
> ==============
> 
> Legend:
> [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
> [ ] = Manual review needed
> 
> 
> Issues:
> =======
> - Header files in -devel subpackage, if present.
>   **Lots of leftover header files in subdirs of /usr/msp430-elf**
>   See: https://docs.fedoraproject.org/en-US/packaging-
>   guidelines/#_devel_packages

It's a compiler, the headers are necessary as far as I'm aware.

> - Package does not contain any libtool archives (.la)
>   Note: msp430-elf-gcc :
>   /usr/msp430-elf/libexec/gcc/msp430-elf/9.2.0/liblto_plugin.la
>   See: https://docs.fedoraproject.org/en-US/packaging-
>   guidelines/#packaging-static-libraries

Fixed.

> - Package uses either %{buildroot} or $RPM_BUILD_ROOT
>   Note: Using both %{buildroot} and $RPM_BUILD_ROOT
>   See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros

I'm not actually using the $RPM_BUILD_ROOT macro, I'm modifying a script that
uses the $RPM_BUILD_ROOT environment variable.  I could switch to using
$RPM_BUILD_ROOT elsewhere in the spec, but then I'm mixing macro formats.

> - 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.
>   Note: License file copying.c is not marked as %license
>   See: https://docs.fedoraproject.org/en-US/packaging-
>   guidelines/LicensingGuidelines/#_license_text

License line was definitely messed up, I fixed it the best I could matching the
gcc, binutils, and gdb packages. I also copied how they handled license files.
I don't understand the comment about copying.c, I'm assuming it's not supposed
to be marked as a license file.

> - Static libraries in -static or -devel subpackage, providing -devel if
>   present.
>   Note: Package has .a files: msp430-elf-gcc, msp430-elf-gcc-c++. Illegal
>   package name: msp430-elf-gcc, msp430-elf-gcc-c++. Does not provide
>   -static: msp430-elf-gcc, msp430-elf-gcc-c++.
>   See: https://docs.fedoraproject.org/en-US/packaging-
>   guidelines/#packaging-static-libraries

Again, this is a compiler, as far as I know, those are necessary.

> 
> 
> ===== MUST items =====
> 
> C/C++:
> [x]: Package does not contain kernel modules.
> [?]: Package contains no static executables.
> [!]: 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.

I agree this is gross, but it's not in "regular" gcc's ld path, so I think it's
okay?

> [x]: Provides: bundled(gnulib) in place as required.
> [x]: If your application is a C or C++ application you must list a
>      BuildRequires against gcc, gcc-c++ or clang.
> [x]: Rpath absent or only used for internal libs.
> 
> Generic:
> [x]: Package successfully compiles and builds into binary rpms on at least
>      one supported primary architecture.
>      Note: Using prebuilt packages
> [x]: Package is licensed with an open-source compatible license and meets
>      other legal requirements as defined in the legal section of Packaging
>      Guidelines.
> [!]: License field in the package spec file matches the actual license.
>      Note: Cannot run licensecheck: Command 'licensecheck -r
>      /var/lib/mock/fedora-
>      rawhide-x86_64/root/builddir/build/BUILD/msp430-elf-toolchain'
>      returned non-zero exit status 2.
>      Review: Ran licensecheck manually. A lot of files are BSD licensed!
>      This should be included in the "License:" block as "GPL and BSD"

Yeah, the license was definitely wrong. Again, fixed to the best of my ability.

> [!]: License file installed when any subpackage combination is installed.
>      Review: all main packages should include the license file.

All packages do include a license file? Again, I've twiddled with exactly
what's included in the above build.

> [x]: Package requires other packages for directories it uses.
>      Note: No known owner of /usr/msp430-elf/libexec/gcc/msp430-elf/9.2.0,
>      /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0, /usr/msp430-elf/lib,
>      /usr/msp430-elf/libexec/gcc/msp430-elf,
>      /usr/msp430-elf/lib/gcc/msp430-elf, /usr/msp430-elf/libexec/gcc,
>      /usr/msp430-elf/msp430-elf/lib, /usr/msp430-elf/bin,
>      /usr/msp430-elf/lib/gcc, /usr/msp430-elf/msp430-elf, /usr/msp430-elf,
>      /usr/msp430-elf/libexec, /usr/msp430-elf/msp430-elf/include

Fixed.

> [!]: Package must own all directories that it creates.
>      Note: Directories without known owners:
>      /usr/msp430-elf/msp430-elf/lib/large/full-memory-range,
>      /usr/msp430-elf/libexec/gcc/msp430-elf, /usr/msp430-elf/libexec/gcc,
>      /usr/msp430-elf/msp430-elf/lib/large/exceptions,
>      /usr/msp430-elf/lib/gcc, /usr/msp430-elf,
>      /usr/msp430-elf/msp430-elf/lib/exceptions,
>      /usr/msp430-elf/msp430-elf/lib/large/full-memory-range/exceptions,
>      /usr/msp430-elf/msp430-elf/include,
>      /usr/msp430-elf/msp430-elf/lib/430/exceptions,
>      /usr/msp430-elf/libexec/gcc/msp430-elf/9.2.0,
>      /usr/msp430-elf/msp430-elf/lib/430,
>      /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0, /usr/msp430-elf/lib,
>      /usr/msp430-elf/lib/gcc/msp430-elf, /usr/msp430-elf/bin,
>      /usr/msp430-elf/msp430-elf/lib, /usr/msp430-elf/msp430-elf/lib/large,
>      /usr/msp430-elf/msp430-elf, /usr/msp430-elf/libexec

Fixed.

> [x]: %build honors applicable compiler flags or justifies otherwise.
> [x]: Package contains no bundled libraries without FPC exception.
> [!]: Changelog in prescribed format.
>      Macros in changelog are not allowed.

I'm not actually using the macro, it's just a comment. Should I escape it
somehow? Or just modify the comment.

> [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
> [?]: 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.
> [?]: 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.
> [?]: Useful -debuginfo package or justification otherwise.
> [x]: Package is not known to require an ExcludeArch tag.
> [-]: 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.
> [!]: Package complies to the Packaging Guidelines
> [x]: Package installs properly.
> [x]: Rpmlint is run on all rpms the build produces.
>      Note: There are rpmlint messages (see attachment).
> [x]: Package does not own files or directories owned by other packages.
> [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]: Reviewer should test that the package builds in mock.
> [!]: Sources can be downloaded from URI in Source: tag
>      Note: Could not download Source0: http://software-
>     
> dl.ti.com/msp430/msp430_public_sw/mcu/msp430/MSPGCC/9_2_0_0/export/msp430-
> gcc-9.2.0.50-source-
>      full.tar.bz2
>      See: https://docs.fedoraproject.org/en-US/packaging-
>      guidelines/SourceURL/
>      NOt
> [-]: 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).
> [!]: Fully versioned dependency in subpackages if applicable.
>      Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in
>      msp430-elf-gcc , msp430-elf-gcc-c++ , msp430-elf-gdb , msp430-elf-
>      binutils
>      Review: it's a good idea to include this.

I'm not sure I understand. I've added some missing requires (the C++ compiler
now requires the C compiler, the C compiler now requires binutils), but if I
add the "Requires: {name}%{?_isa} = %{version}-%{release}" requested, I just
get packages that are uninstallable. Is there some documentation I can look at
to see an example of what's being requested?

> [?]: 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
> [-]: 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]: Package should compile and build into binary rpms on all supported
>      architectures.
> [-]: %check is present and all tests pass.
> [!]: Packages should try to preserve timestamps of original installed
>      files.

Fixed.

> [x]: Spec use %global instead of %define unless justified.
>      Note: %define requiring justification: %define __os_install_post .
>      ./os_install_post
> [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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
> [x]: SourceX is a working URL.
> 
> ===== EXTRA items =====
> 
> Generic:
> [x]: Rpmlint is run on debuginfo package(s).
>      Note: There are rpmlint messages (see attachment).
> [x]: Rpmlint is run on all installed packages.
>      Note: No rpmlint messages.
> [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.
> 
[Snip]


-- 
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
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux