This is my first package, so I am in need of a sponsor. As you can see by the fact this isn't in my Description, I have already screwed up this process, I apologize.

This package has successfully built on Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=14673869

Some quick drive-by comments:

I think the package name should be at least msp430-gcc if not msp430-elf-gcc. If you insist on having gcc as a subpackage then maybe name the source package msp430-elf-toolchain, but I think that's redundant.

%clean section is redundant.

rm -rf %{buildroot} is redundant at the beginning of %install, too.

How are these sources different from what's in Fedora? I can see they are based on gcc-4.9.1, so it would be nice to see the diff between gcc-4.9.1 and the included sources.

A lot of code is bundled, some of which is also packaged in Fedora already (binutils, dejagnu, elfcpp, gas, gcc, gdb, gmp, gold, gprof, itcl, libmpc, mpfr, tcl, tk, zlib and all the lib* directories in mspgcc/sources/tools).

Those that are not used during build should be deleted in %prep and those that can't be unbundled should be declared using Provides: bundled(foo) = version.

New spec URL: https://bitbucket.org/nielsenb/mspgcc-fedora/raw/3b5371a0a86ce831c5a97deca058f314f3f991e3/msp430-elf-toolchain.spec

New SRPM URL: https://bitbucket.org/nielsenb/mspgcc-fedora/downloads/msp430-elf-toolchain-3.5.0.0-1.src.rpm

(In reply to Dominik 'Rathann' Mierzejewski from comment #2)
> Some quick drive-by comments:
> 
> I think the package name should be at least msp430-gcc if not
> msp430-elf-gcc. If you insist on having gcc as a subpackage then maybe name
> the source package msp430-elf-toolchain, but I think that's redundant.
> 

I went with msp430-elf-toolchain for now, since the TI upstream ships gdb with the gcc source.

> 
> %clean section is redundant.
> 

Fixed.

> 
> rm -rf %{buildroot} is redundant at the beginning of %install, too.
> 

Fixed.

> 
> How are these sources different from what's in Fedora? I can see they are
> based on gcc-4.9.1, so it would be nice to see the diff between gcc-4.9.1
> and the included sources.
> 

I'll look into this more. It's a little hard to easily get a diff that makes sense since TI bundles so much code.

> 
> A lot of code is bundled, some of which is also packaged in Fedora already
> (binutils, dejagnu, elfcpp, gas, gcc, gdb, gmp, gold, gprof, itcl, libmpc,
> mpfr, tcl, tk, zlib and all the lib* directories in mspgcc/sources/tools).
> 
> Those that are not used during build should be deleted in %prep and those
> that can't be unbundled should be declared using Provides: bundled(foo) =
> version.
> 

I now remove everything that isn't required for building as Fedora provides the necessary headers. I also remove everything that I'm pretty sure isn't commonly used when doing MSP430 development.

I'm a little confused about adding 'Provides'. Would I do Provides: bundled(msp430-elf-foo) = version? The version of foo I'd provide isn't general use, the object files would be specific to msp430 build targets.

Would I instead be better served to add an msp430-elf-foo subpackage? For example, look at how libgcc is handled in the current gcc specfile.

Created attachment 1176997 [details]
Diff of gcc-4.9.1 vs msp430-gcc 3.5.0.0

Recursive diff of plain gcc-4.9.1 upstream sources vs. the source packaged in msp430-elf-toolchain 3.5.0.0, which is based on the gcc-4.9.1 upstream with modifications by TI / Red Hat.

Hi Brandon, would you please consider unofficially reviewing other review requests to show that you know the packaging guidelines and link the reviews here?

Did a few informal reviews:

https://bugzilla.redhat.com/show_bug.cgi?id=1387927
https://bugzilla.redhat.com/show_bug.cgi?id=1378095
https://bugzilla.redhat.com/show_bug.cgi?id=1382850

New spec URL: https://bitbucket.org/nielsenb/mspgcc-fedora/raw/02c17fe25211ee2243391d146649d2a9b40bb203/msp430-elf-toolchain.spec

New SRPM URL: https://bitbucket.org/nielsenb/mspgcc-fedora/downloads/msp430-elf-toolchain-3.5.0.0-3.src.rpm

No longer assume manpages will be gzipped, as per packaging guidelines. Fixes an improper library strip.

Since the old packages still exist, would it be helpful to mark these as Obsolete-ing the others?

(In reply to Elliott Sales de Andrade from comment #8)
> Since the old packages still exist, would it be helpful to mark these as
> Obsolete-ing the others?

That's definitely a good idea.

Upstream changed how the project is distributed and built back in January and I'm only just getting around to changing my specfile to work with the new versions.

I'll hopefully have an updated specfile and SRPM ready to go soon.

New spec URL: https://bitbucket.org/nielsenb/mspgcc-fedora/raw/91abb9d0f9654defd6c5538cf193b5ae3abd889f/msp430-elf-toolchain.spec

New SRPM URL: https://bitbucket.org/nielsenb/mspgcc-fedora/downloads/msp430-elf-toolchain-5.0.0.0-1.src.rpm

msp430-elf-gcc now obsoletes msp430-gcc. Rebased to the TI / Somnium upstream.

Bit weird that it's version 5.0.0, but 6.2.1 of gcc (and presumably some other
version of the other tools). Not sure what should be done about that.

Minor things out of the spec: Group tag and %defattr are no longer needed; g++ stuff should be its own subpackage, no?

Some issues I see out of fedora-review:

- Provides: bundled(gnulib) in place as required.
  Note: Bundled gnulib but no Provides: bundled(gnulib)
  See:
  http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Requirement_if_you_bundle

- Package does not contain any libtool archives (.la)
  Note: msp430-elf-gcc :
  /usr/lib64/gcc/msp430-elf/6.2.1/plugin/libcc1plugin.la msp430-elf-gcc :
  /usr/libexec/gcc/msp430-elf/6.2.1/liblto_plugin.la msp430-elf-gcc :
  /usr/msp430-elf/lib/430/libssp.la msp430-elf-gcc :
  /usr/msp430-elf/lib/430/libssp_nonshared.la msp430-elf-gcc :
  /usr/msp430-elf/lib/430/libstdc++.la msp430-elf-gcc :
  /usr/msp430-elf/lib/430/libsupc++.la msp430-elf-gcc :
  /usr/msp430-elf/lib/large/libssp.la msp430-elf-gcc :
  /usr/msp430-elf/lib/large/libssp_nonshared.la msp430-elf-gcc :
  /usr/msp430-elf/lib/large/libstdc++.la msp430-elf-gcc :
  /usr/msp430-elf/lib/large/libsupc++.la msp430-elf-gcc :
  /usr/msp430-elf/lib/libssp.la msp430-elf-gcc :
  /usr/msp430-elf/lib/libssp_nonshared.la msp430-elf-gcc :
  /usr/msp430-elf/lib/libstdc++.la msp430-elf-gcc :
  /usr/msp430-elf/lib/libsupc++.la
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries

- SHOULD items: Generic:
[!]: Uses parallel make %{?_smp_mflags} macro.
[!]: 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/latest/exports/msp430-gcc-6.2.1
     .16_source-full.tar.bz2
     See: http://fedoraproject.org/wiki/Packaging:Guidelines#Tags

These match gcc, so probably fine, but I did not check every file:
- Header files in -devel subpackage, if present.
- Static libraries in -static or -devel subpackage, providing -devel if
  present.
  Note: Package has .a files: msp430-elf-gcc. Illegal package name: msp430-elf-gcc. Does not provide -static: msp430-elf-gcc.
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries



Possibly relevant things from rpmlint:
msp430-elf-toolchain-debuginfo.x86_64: E: debuginfo-without-sources
msp430-elf-gcc.x86_64: W: obsolete-not-provided msp430-gcc
msp430-elf-toolchain.src:42: W: unversioned-explicit-obsoletes msp430-gcc

New spec URL: https://bitbucket.org/nielsenb/mspgcc-fedora/raw/d0f1b3ccb1611c8adf5aacf0f63e24444681d9d5/msp430-elf-toolchain.spec

New SRPM URL: https://bitbucket.org/nielsenb/mspgcc-fedora/downloads/msp430-elf-toolchain-5.0.0.0-1.src.rpm

(In reply to Elliott Sales de Andrade from comment #11)
> Bit weird that it's version 5.0.0, but 6.2.1 of gcc (and presumably some
> other
> version of the other tools). Not sure what should be done about that.
> 

I agree, their versioning scheme is incredibly confusing. Doubly so since they changed from the Red Hat developed SDK to the new SOMNIUM one, some version numbers have actually gone backwards. Since we haven't had the packages in Fedora, it hasn't been an issue, but it did break my Copr for awhile until I noticed. I'm not really positive which upstream we should use version numbers for, upstream's upstream (GCC), or just upstream (SOMNIUM).

> Minor things out of the spec: Group tag and %defattr are no longer needed;
> g++ stuff should be its own subpackage, no?
> 

Got rid of the group tag and %defattr. g++ stuff was included in the now dead msp430-gcc, I started out trying to match that as much as possible. I'm open to changing it. Do you know of an example I could look at?

> Some issues I see out of fedora-review:
> 
> - Provides: bundled(gnulib) in place as required.
>   Note: Bundled gnulib but no Provides: bundled(gnulib)
>   See:
>  
> http://fedoraproject.org/wiki/Packaging:
> No_Bundled_Libraries#Requirement_if_you_bundle
> 

Fixed (I think). The bundled(gnulib) needs to be versioned, but which version do I specify?

> - Package does not contain any libtool archives (.la)
>   Note: msp430-elf-gcc :
>   /usr/lib64/gcc/msp430-elf/6.2.1/plugin/libcc1plugin.la msp430-elf-gcc :
>   /usr/libexec/gcc/msp430-elf/6.2.1/liblto_plugin.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/430/libssp.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/430/libssp_nonshared.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/430/libstdc++.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/430/libsupc++.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/large/libssp.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/large/libssp_nonshared.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/large/libstdc++.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/large/libsupc++.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/libssp.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/libssp_nonshared.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/libstdc++.la msp430-elf-gcc :
>   /usr/msp430-elf/lib/libsupc++.la
>   See: http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries
> 

Fixed.

> - SHOULD items: Generic:
> [!]: Uses parallel make %{?_smp_mflags} macro.
> [!]: 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/latest/exports/msp430-
> gcc-6.2.1
>      .16_source-full.tar.bz2
>      See: http://fedoraproject.org/wiki/Packaging:Guidelines#Tags
> 

I now use parallel make, the download link works for me, so not sure what to make of that.

> These match gcc, so probably fine, but I did not check every file:
> - Header files in -devel subpackage, if present.
> - Static libraries in -static or -devel subpackage, providing -devel if
>   present.
>   Note: Package has .a files: msp430-elf-gcc. Illegal package name:
> msp430-elf-gcc. Does not provide -static: msp430-elf-gcc.
>   See: http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries
> 

Anything specific I should do to address these? I'm particular worried about the illegal package name, especially since we've already looked at naming once.

> Possibly relevant things from rpmlint:
> msp430-elf-toolchain-debuginfo.x86_64: E: debuginfo-without-sources
> msp430-elf-gcc.x86_64: W: obsolete-not-provided msp430-gcc
> msp430-elf-toolchain.src:42: W: unversioned-explicit-obsoletes msp430-gcc
>

I don't see the debuginfo-without-sources warning on my system.

msp430-gcc isn't packaged in Fedora 26, and I used an unversioned obsolete since this supersedes msp430-gcc in every way.

For g++ subpackage, I'd check original gcc, maybe.  Same for gnulib, assuming it has also been bundling it. The illegal package name has to do with including static files in a package name without -static suffix, which is likely not relevant here.

I'd say pinging someone from the Embedded SIG might be useful:
https://fedoraproject.org/wiki/Embedded

(In reply to Elliott Sales de Andrade from comment #13)
> For g++ subpackage, I'd check original gcc, maybe.  Same for gnulib,
> assuming it has also been bundling it. The illegal package name has to do
> with including static files in a package name without -static suffix, which
> is likely not relevant here.
> 
> I'd say pinging someone from the Embedded SIG might be useful:
> https://fedoraproject.org/wiki/Embedded

New spec URL: https://bitbucket.org/nielsenb/mspgcc-fedora/raw/e291164cb85b8d82603e5e4f2dc04929f8d5a146/msp430-elf-toolchain.spec

New SRPM URL: https://bitbucket.org/nielsenb/mspgcc-fedora/downloads/msp430-elf-toolchain-5.0.0.0-1.src.rpm

I split out the C++ stuff. Original gcc doesn't bundle gnulib, but the bundled libiberty is not versioned.

I'll check out the Embedded SIG when I get a chance.

So a couple things:
* the URL in the spec does not work anymore,
* the build fails due to missing debugsource files; this is probably related to the debuginfo/debugsource split and not too difficult to fix.

Any chance to ping the Embedded SIG?

(In reply to Elliott Sales de Andrade from comment #15)
> So a couple things:
> * the URL in the spec does not work anymore,
> * the build fails due to missing debugsource files; this is probably related
> to the debuginfo/debugsource split and not too difficult to fix.
> 
> Any chance to ping the Embedded SIG?

Upstream changed things around again, it's being maintained by Mitto Systems Limited now, the earlier version was maintained by Somnium. The URLs got shuffled around in the changeover.

I've only just got my head wrapped around the changes and started updating the specfile. I'll ping the Embedded SIG for their thoughts once I'm done.

New spec URL: https://bitbucket.org/nielsenb/mspgcc-fedora/raw/c1bb3dd343d496c17d7adf11c8df7b014d8cb12e/msp430-elf-toolchain.spec

New SRPM URL: https://bitbucket.org/nielsenb/mspgcc-fedora/downloads/msp430-elf-toolchain-6.0.1.0-1.src.rpm

Rpmlint still shows the download link as broken, but I believe that's because it doesn't follow redirects? TI appears to redirect twice before the actual download.

I see the following during the build:

extracting debug info from /home/nielsenb/rpmbuild/BUILDROOT/msp430-elf-toolchain-6.0.1.0-1.x86_64/usr/lib64/gcc/msp430-elf/7.3.1/plugin/libcp1plugin.so.0.0.0
/usr/lib/rpm/sepdebugcrcfix: Updated 35 CRC32s, 0 CRC32s did match.
cpio: msp430-gcc-7.3.1.24-source-full/build/binutils/binutils/arlex.c: Cannot stat: No such file or directory
cpio: msp430-gcc-7.3.1.24-source-full/build/binutils/binutils/arlex.l: Cannot stat: No such file or directory
cpio: msp430-gcc-7.3.1.24-source-full/build/binutils/binutils/arparse.c: Cannot stat: No such file or directory
cpio: msp430-gcc-7.3.1.24-source-full/build/binutils/binutils/arparse.h: Cannot stat: No such file or directory
cpio: msp430-gcc-7.3.1.24-source-full/build/binutils/binutils/arparse.y: Cannot stat: No such file or directory
cpio: msp430-gcc-7.3.1.24-source-full/build/binutils/ld/ldgram.c: Cannot stat: No such file or directory
cpio: msp430-gcc-7.3.1.24-source-full/build/binutils/ld/ldgram.h: Cannot stat: No such file or directory
cpio: msp430-gcc-7.3.1.24-source-full/build/binutils/ld/ldgram.y: Cannot stat: No such file or directory
cpio: msp430-gcc-7.3.1.24-source-full/build/binutils/ld/ldlex.c: Cannot stat: No such file or directory
cpio: msp430-gcc-7.3.1.24-source-full/build/binutils/ld/ldlex.l: Cannot stat: No such file or directory
cpio: msp430-gcc-7.3.1.24-source-full/build/gcc/gcc/cfns.gperf: Cannot stat: No such file or directory
cpio: msp430-gcc-7.3.1.24-source-full/build/gdb/opcodes/msp430-decode.opc: Cannot stat: No such file or directory
149256 blocks

The resulting compiler seems to work, and it's not clear to me what step of the build is generating the errors.

Finally, poked my head into the Embedded SIG IRC channel, but it was just me and ChanServ. I'll check in again when I have some free time.

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

I am still interested in packaging this software, though the upstream support situation certainly hasn't changed much, with other parts of the toolchain moving back to closed source...

Anyway, I've started work on getting the latest version to build in my copr[0], I will report back with an updated spec and SRPM once I have that working.

0 - https://copr.fedorainfracloud.org/coprs/nielsenb/msp430-development-tools/build/1545336/

New spec URL: https://copr-dist-git.fedorainfracloud.org/cgit/nielsenb/msp430-development-tools/msp430-elf-toolchain.git/plain/msp430-elf-toolchain.spec?id=a1b5d4b24db937c84945ddbeda43545e2ea6cc83

New SRPM URL: https://copr-be.cloud.fedoraproject.org/results/nielsenb/msp430-development-tools/srpm-builds/01550916/msp430-elf-toolchain-9.2.0.0-1.fc32.src.rpm

This is the latest upstream source. The messages from comment 17 still appear. Also note that the strip workaround[0] for msp430 compiled files has changed somewhat, it is now dependent on bash, and now assumes the file passed to the __strip macro is the last argument, not the second. This works around issues caused by the introduction of brp-strip-lto in F33+[1].

I also removed the obsoletes because msp430-gcc has been retired for more Fedora releases than are officially supported so it felt unnecessary, and rpmlint complained about it being unversioned.

0 - https://bugzilla.redhat.com/show_bug.cgi?id=1175942#c6
1 - https://bugzilla.redhat.com/show_bug.cgi?id=1789099

I see this has been opened for a longer while so I'll take it :).

I can't download the sources atm, because TI is a doing scheduled maintenance, but I had a quick look at the SPEC file:

rpmlint shows the following:
msp430-elf-toolchain.spec:51: W: unversioned-explicit-provides bundled(gnulib)
msp430-elf-toolchain.spec:295: W: macro-in-%changelog %{optflags}
msp430-elf-toolchain.spec: W: invalid-url Source0: http://software-dl.ti.com/msp430/msp430_public_sw/mcu/msp430/MSPGCC/9_2_0_00/exports/msp430-gcc-9.2.0.50-source-full.tar.bz2 HTTP Error 404: Not Found (because the URL is unreachable?)

> URL:		http://software-dl.ti.com/msp430/msp430_public_sw/mcu/msp430/MSPGCC/%{ti_version_number}/index_FDS.html
> Source0:	http://software-dl.ti.com/msp430/msp430_public_sw/mcu/msp430/MSPGCC/%{ti_version_number}/exports/%{archive_name}.tar.bz2

The URL can probably be reused in the Source0 link as %{url} for brevity.

> Source1:	README.fedora
> BuildRequires:	bash
> BuildRequires:	gcc
> [...]

If possible, leave an empty line between SourceN and BuildRequires, and sort BuildRequires alphabetically.

> %package -n msp430-elf-gcc
> Summary: GCC for the MSP430
> Group: Development/Debuggers
> Provides: bundled(gnulib)

The Group tag is deprecated and should not be used.

> %package -n msp430-elf-gcc-c++
> Summary: GCC for the MSP430
> Group: Development/Debuggers
> %description -n msp430-elf-gcc-c++
> This is a cross compiling version of g++, which can be used to
> compile for the %{target} platform, instead of for the native %{_arch}
> platform.

An empty line before the %description in these package blocks would be nice :).

> cd binutils
> ../../binutils/configure --prefix=%{_prefix} --libdir=%{_libdir} %configure_args_common %configure_args_binutils
> make %{?_smp_mflags}
> cd -

Use the %make_build macro instead of make %{?_smp_mflags} where applicable.

> cd binutils
> make install DESTDIR=%{buildroot}
> cd -

Use the %make_install macro instead if possible.

I'll run licensecheck and go through the full review matrix once I have a bit more time and access to the sources.

Some of the URLs have moved slightly (or were maybe wrong from the beginning), those will be fixed in the next revision.

Right now I'm trying to figure out how to get it to stop installing things in /usr/msp430-elf, which is obviously a non-starter. I've made some progress on that front, but some intermediates seem to wind up there.

Follow up, I've confused myself, and could use guidance. The documentation for packaging cross compilers[0] states "All cross-compilers should add --prefix=/usr/arch-os-libc to ./configure when building the toolchain. This is according to the cross-compiling guidelines in GCC's INSTALL document." I cannot find this guideline in the GCC documentation[1]. I can do this of course, but it results in /usr/msp430-elf/bin, /usr/msp430-elf/lib, etc... I have experimented with this, and with symlinking the resulting binaries into /usr/bin (as noted in the Fedora documentation), the compiler works as expected.

That being said, that it not how the most similar cross compiler, avr-gcc, is packaged[2].

So, I count 3 paths forward:

1: Specify prefix as per Fedora guidelines
2: Try to match avr-gcc, with everything going roughly where I would expect, binaries in /usr/bin, library files in /usr/lib/gcc/avr, etc...
3: The terrible hybrid I've done in all the builds above, with binaries going in /usr/bin, libraries and include files mostly going in /usr/msp430-elf/lib, but some going in /usr/lib...

I really don't like 3, and I regret ever doing it. In my haste to get a working package together I went with the first thing that worked. I've been trying to do 2, but I'll be darned if I can get a working compiler out of it. 1 is easy, and I've proved it works, but it feels "weird".

0 - https://fedoraproject.org/wiki/Packaging_Cross_Compiling_Toolchains#Cross-compiling_GCC_tool-chains
1 - https://gcc.gnu.org/install/configure.html 
2 - https://src.fedoraproject.org/rpms/avr-gcc

> Some of the URLs have moved slightly (or were maybe wrong from the beginning), those will be fixed in the next revision.

I managed to find the latest sources here: 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
I think one of your %global definitions should be:
%global ti_version_number 9_2_0_0
instead of:
%global ti_version_number 9_2_0_00
And the URL block should be a little different.

> The documentation for packaging cross compilers[0] states "All cross-compilers should add --prefix=/usr/arch-os-libc to ./configure when building the toolchain. This is according to the cross-compiling guidelines in GCC's INSTALL document." I cannot find this guideline in the GCC documentation[1]. I can do this of course, but it results in /usr/msp430-elf/bin, /usr/msp430-elf/lib, etc... I have experimented with this, and with symlinking the resulting binaries into /usr/bin (as noted in the Fedora documentation), the compiler works as expected.

I would stick to the official Fedora packaging docs in this case. I'm not sure why avr-gcc uses `--prefix=%{_prefix}`, but the docs you linked also mention avr-gcc is a bit of a special case. Unfortunately, I don't have much experience with packaging compilers.

> So, I count 3 paths forward:
> 
> 1: Specify prefix as per Fedora guidelines
> 2: Try to match avr-gcc, with everything going roughly where I would expect, binaries in /usr/bin, library files in /usr/lib/gcc/avr, etc...
> 3: The terrible hybrid I've done in all the builds above, with binaries going in /usr/bin, libraries and include files mostly going in /usr/msp430-elf/lib, but some going in /usr/lib...

I would go with 1. in this case. Putting everything into a prefixed dir inside `/usr` (`/usr/msp430-elf`, for instance) and then symlinking seems like a cleaner and better self-contained approach than symlinking some of the files and directly putting the rest into regular directories.

Spec URL: https://copr-be.cloud.fedoraproject.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01565363-msp430-elf-toolchain/msp430-elf-toolchain.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/nielsenb/msp430-development-tools/srpm-builds/01565363/msp430-elf-toolchain-9.2.0.0-2.fc32.src.rpm

This hopefully addresses the issues from comment 21, as well as changing to specifying --prefix=/usr/msp430-elf as discussed in comment 23 and comment 24. The more I think about it, the more that's absolutely the right way to do it. The only oddity is a lot of the actual headers and libraries for the target end of in /usr/msp430-elf/msp430-elf, which isn't an issue, just a little ugly.

For some reason it fails to build for Fedora 32 on aarch64[0], but I was able to do the same build with mock locally, so I'm going to assume it was an issue on the copr end.

0 - https://copr.fedorainfracloud.org/coprs/nielsenb/msp430-development-tools/build/1565812/

The SPEC file looks better, but I'm having some issues trying to build it. `fedora-pkg` refuses to download the source tarball, even though `wget` gets it without issues. When running a scratch build in koji, I unfortunately see this:
https://koji.fedoraproject.org/koji/taskinfo?taskID=47582012

I'll try to fiddle with this a bit more and let you know.

(In reply to Andy Mender from comment #26)
> The SPEC file looks better, but I'm having some issues trying to build it.
> `fedora-pkg` refuses to download the source tarball, even though `wget` gets
> it without issues. When running a scratch build in koji, I unfortunately see
> this:
> https://koji.fedoraproject.org/koji/taskinfo?taskID=47582012
> 
> I'll try to fiddle with this a bit more and let you know.

wget follows redirects, other parts of the rpm build chain don't seem to, though rpmlint seems to now.

The koji build failure looks like an issue with my workaround for the changes to strip in F33[0], interesting that it seemed to work in copr? I'll dig into it more.

[0] - https://bugzilla.redhat.com/show_bug.cgi?id=1789099

My strip "wrapper" (for want of a better term) isn't called on F33. I might try to port the workaround used for avr-gcc since it looks like it's lower maintenance than my solution, and actually works. I'm open to any other suggestions as well.

Spec URL: https://copr-be.cloud.fedoraproject.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01575217-msp430-elf-toolchain/msp430-elf-toolchain.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/nielsenb/msp430-development-tools/srpm-builds/01575217/msp430-elf-toolchain-9.2.0.0-2.fc32.src.rpm

Okay, this works everywhere I've tried it[0]. Further feedback is welcome, and encouraged!

[0] - https://copr.fedorainfracloud.org/coprs/nielsenb/msp430-development-tools/build/1575217/

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.

> 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.

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):

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
- 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
- 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
- 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
- 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


===== 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.
[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"
[!]: License file installed when any subpackage combination is installed.
     Review: all main packages should include the license file.
[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
[!]: 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
[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.
[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.
[?]: 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.
[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.


**Full rpmlint in attached file**



Unversioned so-files
--------------------
msp430-elf-gcc: /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/plugin/libcc1plugin.so
msp430-elf-gcc: /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/plugin/libcp1plugin.so
msp430-elf-gcc: /usr/msp430-elf/libexec/gcc/msp430-elf/9.2.0/liblto_plugin.so

Requires
--------
msp430-elf-gcc (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libcc1plugin.so.0()(64bit)
    libcp1plugin.so.0()(64bit)
    libdl.so.2()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_3.3)(64bit)
    libgcc_s.so.1(GCC_3.4)(64bit)
    libgcc_s.so.1(GCC_4.2.0)(64bit)
    libgmp.so.10()(64bit)
    liblto_plugin.so.0()(64bit)
    libm.so.6()(64bit)
    libmpc.so.3()(64bit)
    libmpfr.so.6()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.8)(64bit)
    libstdc++.so.6(CXXABI_1.3.9)(64bit)
    rtld(GNU_HASH)

msp430-elf-gcc-c++ (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.3)(64bit)
    libgcc_s.so.1(GCC_4.2.0)(64bit)
    libm.so.6()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.9)(64bit)
    rtld(GNU_HASH)

msp430-elf-gdb (rpmlib, GLIBC filtered):
    /usr/bin/sh
    libc.so.6()(64bit)
    libdl.so.2()(64bit)
    libexpat.so.1()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libm.so.6()(64bit)
    libncursesw.so.6()(64bit)
    libpthread.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.11)(64bit)
    libstdc++.so.6(CXXABI_1.3.2)(64bit)
    libstdc++.so.6(CXXABI_1.3.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.5)(64bit)
    libstdc++.so.6(CXXABI_1.3.8)(64bit)
    libstdc++.so.6(CXXABI_1.3.9)(64bit)
    libtinfo.so.6()(64bit)
    rtld(GNU_HASH)

msp430-elf-binutils (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libdl.so.2()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.3)(64bit)
    libgcc_s.so.1(GCC_4.2.0)(64bit)
    libm.so.6()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.9)(64bit)
    rtld(GNU_HASH)

msp430-elf-toolchain-debuginfo (rpmlib, GLIBC filtered):

msp430-elf-toolchain-debugsource (rpmlib, GLIBC filtered):



Provides
--------
msp430-elf-gcc:
    bundled(gnulib)
    libcc1plugin.so.0()(64bit)
    libcp1plugin.so.0()(64bit)
    liblto_plugin.so.0()(64bit)
    msp430-elf-gcc
    msp430-elf-gcc(x86-64)

msp430-elf-gcc-c++:
    msp430-elf-gcc-c++
    msp430-elf-gcc-c++(x86-64)

msp430-elf-gdb:
    msp430-elf-gdb
    msp430-elf-gdb(x86-64)

msp430-elf-binutils:
    msp430-elf-binutils
    msp430-elf-binutils(x86-64)

msp430-elf-toolchain-debuginfo:
    msp430-elf-toolchain-debuginfo
    msp430-elf-toolchain-debuginfo(x86-64)

msp430-elf-toolchain-debugsource:
    msp430-elf-toolchain-debugsource
    msp430-elf-toolchain-debugsource(x86-64)

Created attachment 1702414 [details]
FUll review

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]

Macros are never ignored, whether in comments or changelog. You need to double the % in those cases, or leave it out entirely.

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

No worries then.


> 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.
> It's a compiler, the headers are necessary as far as I'm aware.

Unfortunately, I don't have much experience with packaging entire compiler toolchains. I'll ask in the fedora-devel mailing list to get some extra intel.

> 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.

I know. I just left it in as it's a part of the review matrix.

> 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.

I saw this in a couple of other C/C++ packages as well. The file does actually contain the license, but it's not a license file per-se. It merely generates it via consecutive print calls.

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

I think it's okay.

> 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?

Here's the section from the Packaging Guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package
It typically applies to situations in which you have a base package and multiple subpackages. I might be misunderstanding this as well, but I think `fedora-review` here expects that every component should be tied to the main %{target}-toolchain package. But then again, we want some packages to be installable independently, but only be built together as a complete toolchain, right?

(In reply to Brandon Nielsen from comment #32)
> 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


When creating the symlinks for the binutils executables I'd be tempted to do:


# Symlink binutils so we can build GCC
mkdir bin
for i in addr2line ar cxxfilt elfedit gprof libtool objcopy objdump ranlib readelf size strings
do
  ln -sr binutils/binutils/$i bin/%{target}-$i
done
for i in as ld nm strip
do
  ln -sr binutils/binutils/$i-new bin/%{target}-$i
done

Personally I find it a bit easier to read because it's less of a wall of text, and it's clear which ones are linked using the same name and which ones aren't. But that's just me, what you have is fine.

I don't think building GCC actually needs most of these, but it doesn't hurt to add them.


> > 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.

Right.

> 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.

It's possible the msp430-elf-gcc package won't actually work without some of those headers, so there's no benefit to putting them in a separate -devel package.

But it's not true for all of them.

The files in /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/plugin/ are not strictly necessary to use the compiler, only if you want to build GCC plugins for msp430-elf. You could either not bother packaging the plugin support at all, or you could leave it in the main msp430-elf-gcc package (which is what the avr-gcc package appears to do too).

The files in /usr/msp430-elf/msp430-elf/include/ appear to be the newlib libc headers. In theory they could be put in a separate msp430-elf-newlib-devel subpackage. Code which doesn't use anything from the C standard library could still be compiled by msp430-elf-gcc without the newlib headers.


> > - 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.

Again, true for some of those libs that msp430-elf-gcc depends on, but not true for the ones in msp430-elf-gcc-c++.

Fedora's native GCC package puts the C++ standard library headers and libs into separate packages, not part of gcc-c++. Specifically, libstdc++-devel and libstdc++-static.

That means the gcc-c++ package can be used to compile C++ code that doesn't depend on the standard library or the C++ runtime. But for most practical uses you also need to install libstdc++-devel (and for static linking, libstdc++-static as well).


That said, this seems like a reasonable way to split the packages up. There's probably not much practical benefit to more granularity for the stdlib files.



> > 
> > 
> > ===== 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?

Yes, I think these are OK where they are:

msp430-elf-gcc: /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/plugin/libcc1plugin.so
msp430-elf-gcc: /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/plugin/libcp1plugin.so
msp430-elf-gcc: /usr/msp430-elf/libexec/gcc/msp430-elf/9.2.0/liblto_plugin.so

They're specific to this build of GCC, and won't be used by anything else.



> > [!]: 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.

As mentioned in comment 33, either remove it or escape it with %%.


> > [!]: 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?

I think you want %{?_isa} to be present on the Requires tags, otherwise it's possible for a user to manually install the 32-bit msp430-elf-binutils.i686 package on an x86_64 and have that satisfy the requirement for msp430-elf-binutils. But what you're really trying to require is the msp430-elf-binutils.x86_64 one, so you need the %{?_isa} suffix.

I agree that you don't want the subpackages to require the main one, but that's because the main one doesn't actually exist ... no msp430-elf-toolchain package actually gets created.

Shouldn't that main package be an umbrella package that installs the others, i.e. the main package should have:

Requires: %{name}-binutils%{?_isa} = %{version}-%{release}
Requires: %{name}-gcc%{?_isa} = %{version}-%{release}
Requires: %{name}-gcc-c++%{?_isa} = %{version}-%{release}
Requires: %{name}-gdb%{?_isa} = %{version}-%{release}

That would allow you to say 'dnf install msp430-elf-toolchain' and get all of the subpackages at once.

(In reply to Jonathan Wakely from comment #35)
> (In reply to Brandon Nielsen from comment #32)
> > 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
> 
> 
> When creating the symlinks for the binutils executables I'd be tempted to do:
> 
> 
> # Symlink binutils so we can build GCC
> mkdir bin
> for i in addr2line ar cxxfilt elfedit gprof libtool objcopy objdump ranlib
> readelf size strings
> do
>   ln -sr binutils/binutils/$i bin/%{target}-$i
> done
> for i in as ld nm strip
> do
>   ln -sr binutils/binutils/$i-new bin/%{target}-$i
> done
> 
> Personally I find it a bit easier to read because it's less of a wall of
> text, and it's clear which ones are linked using the same name and which
> ones aren't. But that's just me, what you have is fine.
> 
> I don't think building GCC actually needs most of these, but it doesn't hurt
> to add them.
> 

I could feasibly do away with the renaming entirely and use the '--program-prefix' configure option (see additional discussion below)? No matter what, I like your solution better.

[Snip]
> > > 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.
> 
> It's possible the msp430-elf-gcc package won't actually work without some of
> those headers, so there's no benefit to putting them in a separate -devel
> package.
> 
> But it's not true for all of them.
> 
> The files in /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/plugin/ are not
> strictly necessary to use the compiler, only if you want to build GCC
> plugins for msp430-elf. You could either not bother packaging the plugin
> support at all, or you could leave it in the main msp430-elf-gcc package
> (which is what the avr-gcc package appears to do too).
> 
> The files in /usr/msp430-elf/msp430-elf/include/ appear to be the newlib
> libc headers. In theory they could be put in a separate
> msp430-elf-newlib-devel subpackage. Code which doesn't use anything from the
> C standard library could still be compiled by msp430-elf-gcc without the
> newlib headers.
> 

I've waffled back and forth on the plugin headers since I've started trying to package this. Ultimately, the "upstream" (such that it is) binary packages include them last I checked, so I figured I would as well. Same with the newlib headers. I could check again, but at some level, if I'm shipping most of the headers, why not just ship all?
 
> > > - 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.
> 
> Again, true for some of those libs that msp430-elf-gcc depends on, but not
> true for the ones in msp430-elf-gcc-c++.
> 
> Fedora's native GCC package puts the C++ standard library headers and libs
> into separate packages, not part of gcc-c++. Specifically, libstdc++-devel
> and libstdc++-static.
> 
> That means the gcc-c++ package can be used to compile C++ code that doesn't
> depend on the standard library or the C++ runtime. But for most practical
> uses you also need to install libstdc++-devel (and for static linking,
> libstdc++-static as well).
> 
> 
> That said, this seems like a reasonable way to split the packages up.
> There's probably not much practical benefit to more granularity for the
> stdlib files.
> 

avr-gcc doesn't appear to do this, I can check the other cross compilers. I'm open to either really.
 
> 
> > > 
> > > 
> > > ===== 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?
> 
> Yes, I think these are OK where they are:
> 
> msp430-elf-gcc:
> /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/plugin/libcc1plugin.so
> msp430-elf-gcc:
> /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/plugin/libcp1plugin.so
> msp430-elf-gcc: /usr/msp430-elf/libexec/gcc/msp430-elf/9.2.0/liblto_plugin.so
> 
> They're specific to this build of GCC, and won't be used by anything else.
> 

I will remove them.

[Snip]
> > > [!]: 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?
> 
> I think you want %{?_isa} to be present on the Requires tags, otherwise it's
> possible for a user to manually install the 32-bit msp430-elf-binutils.i686
> package on an x86_64 and have that satisfy the requirement for
> msp430-elf-binutils. But what you're really trying to require is the
> msp430-elf-binutils.x86_64 one, so you need the %{?_isa} suffix.
>

Thanks, I understand now.
 
> I agree that you don't want the subpackages to require the main one, but
> that's because the main one doesn't actually exist ... no
> msp430-elf-toolchain package actually gets created.
> 
> Shouldn't that main package be an umbrella package that installs the others,
> i.e. the main package should have:
> 
> Requires: %{name}-binutils%{?_isa} = %{version}-%{release}
> Requires: %{name}-gcc%{?_isa} = %{version}-%{release}
> Requires: %{name}-gcc-c++%{?_isa} = %{version}-%{release}
> Requires: %{name}-gdb%{?_isa} = %{version}-%{release}
> 
> That would allow you to say 'dnf install msp430-elf-toolchain' and get all
> of the subpackages at once.

That's a good idea! My only thought is I don't actually know many developers doing C++ on embedded, but I like the idea of being able to get the complete toolchain.

An additional question, since I've moved to setting the '--prefix=%{_prefix}/%{target}' on configure, I've needed to add a matching '-B/usr/bin/msp430-elf-' to any Makefile using the resulting compiler. Would setting the program prefix, instead of adding the prefix with my symlinks, fix this?

One final question, do I need to run add a check step? They used to not work, but I think they do now.

(In reply to Brandon Nielsen from comment #36)
> > 
> > > > 
> > > > 
> > > > ===== 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?
> > 
> > Yes, I think these are OK where they are:
> > 
> > msp430-elf-gcc:
> > /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/plugin/libcc1plugin.so
> > msp430-elf-gcc:
> > /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/plugin/libcp1plugin.so
> > msp430-elf-gcc: /usr/msp430-elf/libexec/gcc/msp430-elf/9.2.0/liblto_plugin.so
> > 
> > They're specific to this build of GCC, and won't be used by anything else.
> > 
> 
> I will remove them.

What I meant is that they're fine to keep. If you remove them you won't be able to use GDB's on-demand compilation feature, or LTO.

 
> > I agree that you don't want the subpackages to require the main one, but
> > that's because the main one doesn't actually exist ... no
> > msp430-elf-toolchain package actually gets created.
> > 
> > Shouldn't that main package be an umbrella package that installs the others,
> > i.e. the main package should have:
> > 
> > Requires: %{name}-binutils%{?_isa} = %{version}-%{release}
> > Requires: %{name}-gcc%{?_isa} = %{version}-%{release}
> > Requires: %{name}-gcc-c++%{?_isa} = %{version}-%{release}
> > Requires: %{name}-gdb%{?_isa} = %{version}-%{release}
> > 
> > That would allow you to say 'dnf install msp430-elf-toolchain' and get all
> > of the subpackages at once.
> 
> That's a good idea! My only thought is I don't actually know many developers
> doing C++ on embedded,

If they don't want the whole thing they only need to install msp430-elf-gcc (which also installs msp430-elf-binutils).

If they want a debugger too they can install msp430-elf-gdb.

Nobody *has* to use the -toolchain package to get everything :-)

> but I like the idea of being able to get the complete
> toolchain.
> 
> An additional question, since I've moved to setting the
> '--prefix=%{_prefix}/%{target}' on configure, I've needed to add a matching
> '-B/usr/bin/msp430-elf-' to any Makefile using the resulting compiler. Would
> setting the program prefix, instead of adding the prefix with my symlinks,
> fix this?
> 
> One final question, do I need to run add a check step? They used to not
> work, but I think they do now.

I'm not sure what you're referring to, %check steps have always worked as far as I know.

It's probably a good idea to add one. Maybe just compile and link a "hello, world" program as a sanity check. You won't be able to run it of course.

(In reply to Jonathan Wakely from comment #37)
> (In reply to Brandon Nielsen from comment #36)
[Snip]
> > One final question, do I need to run add a check step? They used to not
> > work, but I think they do now.
> 
> I'm not sure what you're referring to, %check steps have always worked as
> far as I know.
> 
> It's probably a good idea to add one. Maybe just compile and link a "hello,
> world" program as a sanity check. You won't be able to run it of course.

That was poorly worded. I used to never be able to get 'make check' to do more than produce a bunch of failures, no matter how I built the upstream sources. It seems to have started working at some point. I'll add the corresponding %check step to the specfile.

(In reply to Brandon Nielsen from comment #36)
[Snip]
> An additional question, since I've moved to setting the
> '--prefix=%{_prefix}/%{target}' on configure, I've needed to add a matching
> '-B/usr/bin/msp430-elf-' to any Makefile using the resulting compiler. Would
> setting the program prefix, instead of adding the prefix with my symlinks,
> fix this?
> 
[Snip]

Answering my own question, setting the '--program-prefix=msp430-elf-' configure option did not stop msp430-elf-gcc from attempting to use unprefixed binutils by default, '-B/usr/bin/msp430-elf-' is still required in the CFLAGS to use the compiler. Wish I could come up with a way to fix that.

Not to wear out my welcome, I have most of the changes above squared away and tested, just trying to figure out why I need to compile programs with the -B flag mentioned above. Not a huge deal, but feels wrong.

In the interim, it was suggested on the devel mailing list[0] that this could instead be built with the cross-gcc[1] and cross-binutils[2] packages. Doing this and having a useful (for embedded development) compiler at the end requires a matching newlib package, as is done for arm[3]. I've mocked all of this up, and while building it is super confusing, it does work. Would that be a better path forward? Do I run it up the flagpole on the devel mailing list? Somewhere else?

[0] - https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/KAYAKXOY7ZPQEENB6PUM2EEUN53X2HQQ/
[1] - https://src.fedoraproject.org/rpms/cross-gcc
[2] - https://src.fedoraproject.org/rpms/cross-binutils
[3] - https://src.fedoraproject.org/rpms/arm-none-eabi-newlib

Yes, the devel list. Make sure you have a sensible subject that will get the attention of compiler/toolchain/cross-compiler people.

I have solicited feedback on the devel list: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/W72GCBDQEN2VHHHMTUB3ALWXVIXLV6RG/

Hello Brandon, any updates on this?

No real progress.

The post to the devel list went nowhere. I have been unable to modify the cross-gcc specfile to build a compiler for the MSP430 that generates a binary that runs on actual hardware. It is possible I'm not correctly compiling newlib. I have not had a chance to compare the resulting assembly versus a working compiler. Trying to get it working consumes a lot of CPU time, actual time, and disk space. I only really have the latter. Given the lack of response I've mostly abandoned the idea for now.

As for this review, I still need to add the check step. I'm also still trying to figure out why the '-B/usr/bin/msp430-elf-' is required. It breaks a lot of existing Makefiles. Hopefully I'll get a chance to dig into that all more this week.

I'm wondering whether perhaps it's not possible to split this into multiple SPEC files and tackle each component separately? For instance, to do msp430-elf-binutils or msp430-elf-gdb first. For instance, here the packager focused on gdb alone: https://bugzilla.redhat.com/show_bug.cgi?id=1859627

(In reply to Andy Mender from comment #45)
> I'm wondering whether perhaps it's not possible to split this into multiple
> SPEC files and tackle each component separately? For instance, to do
> msp430-elf-binutils or msp430-elf-gdb first. For instance, here the packager
> focused on gdb alone: https://bugzilla.redhat.com/show_bug.cgi?id=1859627

The source used for the specfile in this review is _not_ the vanilla GCC / GDB / binutils upstream, it is a bespoke version done by Mitto Systems for TI and ships GCC / GDB / binutils all in one tarball. A lot of the work has been upstreamed, but there are still differences. I could redo the work of comment #4 to document what still differs, but ultimately I think most developers would prefer using something as close as possible to what TI provides, which is why I'm not okay with the current state of the specfile, the resulting binaries would not work with Makefiles targeting the TI provided blobs (which is probably >99% of them). As mentioned above, I've been short on time to investigate this behavior.

Circling back to splitting this into multiple SPEC files, would that be allowed? They would all share the same source file, which seems like it may be confusing. But this SPEC file is getting unwieldy. I also think it would help clean up some of the ugliness I forced on myself by doing a combined tree build.

I have figured out the issue with requiring -B to build using the resulting compiler. The final clue came from a mailing list post[0], basically, I shouldn't have been deleting the unprefixed binaries in /usr/msp430-elf/msp430-elf/bin. Unfortunately they are duplicates of the prefixed binaries, so I guess I'll just symlink the prefixed ones in /usr/msp430-elf/bin to the expected path? Feels gross. There might be a better solution based on --with-as and --with-ld flags[1].

Once I've cleaned up that SPEC file and gotten it building I'll link it up. Still haven't figured out why I can't get make check to work.

Thanks for sticking with this boondoggle.

[0] - https://sourceware.org/pipermail/crossgcc/2001-June/008966.html
[1] - https://gcc.gnu.org/legacy-ml/gcc-help/2006-04/msg00102.html

Apologies for the delay with this!

I went through the various Packaging Guidelines and it doesn't seem like there is anything against splitting the package into multiple SPEC files. The only potential problem it introduces is that certain subpackages might become uninstallable if the dependency is not available yet. On the plus side, maybe when you split the SPEC, you'll notice a pattern that will let you re-assemble everything into a single SPEC file again or you'll realize it wouldn't work in a single SPEC.

> In the interim, it was suggested on the devel mailing list[0] that this could instead be built with the cross-gcc[1] and cross-binutils[2] packages. Doing this and having a useful (for embedded development) compiler at the end requires a matching newlib package, as is done for arm[3]. I've mocked all of this up, and while building it is super confusing, it does work. Would that be a better path forward? Do I run it up the flagpole on the devel mailing list? Somewhere else?

I shortly revisited this bit. To my understanding, cross-gcc and cross-binutils are meant for building OS kernels, not cross-compiling regular userland software. However, following this example might be useful, as suggested on the mailing list: https://src.fedoraproject.org/rpms/arm-none-eabi-newlib/blob/master/f/arm-none-eabi-newlib.spec

However, that still requires the various -binutils, -gcc, etc. packages for MSP430.

(In reply to Andy Mender from comment #48)
> Apologies for the delay with this!
> 
> I went through the various Packaging Guidelines and it doesn't seem like
> there is anything against splitting the package into multiple SPEC files.
> The only potential problem it introduces is that certain subpackages might
> become uninstallable if the dependency is not available yet. On the plus
> side, maybe when you split the SPEC, you'll notice a pattern that will let
> you re-assemble everything into a single SPEC file again or you'll realize
> it wouldn't work in a single SPEC.
> 

At this point the only thing that I don't have working is the GDB check tests. I'll drill down on that next. If I can't get that working I'll look into splitting the package. Remember when I said I have enough disk space? Turns out I don't have that either...

> > In the interim, it was suggested on the devel mailing list[0] that this could instead be built with the cross-gcc[1] and cross-binutils[2] packages. Doing this and having a useful (for embedded development) compiler at the end requires a matching newlib package, as is done for arm[3]. I've mocked all of this up, and while building it is super confusing, it does work. Would that be a better path forward? Do I run it up the flagpole on the devel mailing list? Somewhere else?
> 
> I shortly revisited this bit. To my understanding, cross-gcc and
> cross-binutils are meant for building OS kernels, not cross-compiling
> regular userland software. However, following this example might be useful,
> as suggested on the mailing list:
> https://src.fedoraproject.org/rpms/arm-none-eabi-newlib/blob/master/f/arm-
> none-eabi-newlib.spec
> 
> However, that still requires the various -binutils, -gcc, etc. packages for
> MSP430.

That was my understanding as well, though it was noted it "shouldn't matter"[0]. Ultimately, even if I did get them working, it wouldn't match what TI ships and is discussed on the TI forums.

[0] - https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/KRAG5KT3UTZTMLFT3O53STP2E3H33GXH/

Spec URL: https://download.copr.fedorainfracloud.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01718629-msp430-elf-toolchain/msp430-elf-toolchain.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01718629-msp430-elf-toolchain/msp430-elf-toolchain-9.2.0.0-4.fc34.src.rpm

It's a build, and it works, and it has a check step! Settle in and make yourself comfortable before building this one!

It now carries a patch file, 2 actually, to work around issues with the GDB tests. There are actually lots of failures with the GDB tests, so I limited the suite to gdb.base to make it more workable. There were additional issues with GDB ix86 tests, so that carries an additional patch. I think patching conditionally based on architecture is okay? I looked at the tests that were failing, and to my relatively untrained eye I think the failures make sense for an embedded target. There is probably a "cleaner" way to handle it and I'm open to suggestions. The unfortunate reality of "upstream" just throwing tarballs over the wall is there isn't a good place to resolve these issues.

Additionally, unprefixed symlinks to the executables are now made in /usr/msp430-elf/msp430-elf/bin as their presence is expected by the generated msp430-elf-gcc. I don't fully understand why. This eliminates the need for the -B/usr/bin/msp430-elf- compile option, maintaining better compatibility with Makefiles. Again, I'm sure there's a better way but I don't know it.

Actually, ignore the Spec / SRPM from comment 50, I have a new build going that enables the binutils and gcc tests as well. I'll link them again when it completes. Rest of the comment still applies.

Spec URL: https://download.copr.fedorainfracloud.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01718805-msp430-elf-toolchain/msp430-elf-toolchain.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01718805-msp430-elf-toolchain/msp430-elf-toolchain-9.2.0.0-4.fc34.src.rpm

This time with tests for all three packages enabled.

Extra Koji build from the latest SRPM: https://koji.fedoraproject.org/koji/taskinfo?taskID=55638767

Fails on s390x, but I don't think it's the SRPMs fault.

> It now carries a patch file, 2 actually, to work around issues with the GDB tests. There are actually lots of failures with the GDB tests, so I limited the suite to gdb.base to make it more workable. There were additional issues with GDB ix86 tests, so that carries an additional patch. I think patching conditionally based on architecture is okay?

rpmlint has mixed feelings about this, but I think it's okay:
> msp430-elf-toolchain.src: W: %ifarch-applied-patch Patch1: msp430-elf-toolchain_i386_testfix.patch

I found this, though: https://listman.redhat.com/archives/fedora-devel-list/2007-September/msg01515.html

> Provides: bundled(gnulib)

Didn't notice this before, but rpmlint complains that this should be versioned:
> msp430-elf-toolchain.src:61: W: unversioned-explicit-provides bundled(gnulib)


Which does make sense, because without versioning it's impossible to tell which gnulib it is. Unless, it's not possible to ascertain, because the source is a tarball and not much more.

> case $a in
> # Prevent brp-strip* from trying to handle foreign binaries
> */brp-strip*)
>   b=$(basename $a)
>   sed -e 's,find "*$RPM_BUILD_ROOT"*,find "$RPM_BUILD_ROOT" -not -path "%{buildroot}%{_prefix}/%{target}/lib/gcc/%{target}/%{gcc_version_base}/*" -not -path "%{buildroot}%{_prefix}/%{target}/%{target}/lib/*",' $a > $b
>   chmod a+x $b
>   ;;
> esac
> done

Is it possible to use %{buildroot} instead of $RPM_BUILD_ROOT in the sed call? Both should expand to the same path.

rpmlint also complains about quite a lot of header files in the main subpackages:
> msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libgcc.a
> msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libgcov.a
> msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libmul_16.a
> msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libmul_32.a
> msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libmul_f5.a
> msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libmul_none.a
> [...]

I'm guessing these are needed by the main packages and the lines can be ignored?

Not sure about these, though:
> msp430-elf-gcc.x86_64: W: dangling-relative-symlink /usr/lib/.build-id/3d/b4b5215ce578d5b13456988092f30187ac4beb ../../../../usr/msp430-elf/libexec/gcc/msp430-elf/9.2.0/cc1plus

This symlink doesn't look valid. Is it an artifact from the build?

> msp430-elf-binutils.x86_64: W: non-standard-dir-in-usr msp430-elf

> Additionally, unprefixed symlinks to the executables are now made in /usr/msp430-elf/msp430-elf/bin as their presence is expected by the generated msp430-elf-gcc. I don't fully understand why. This eliminates the need for the -B/usr/bin/msp430-elf- compile option, maintaining better compatibility with Makefiles. Again, I'm sure there's a better way but I don't know it.

Yeah, rpmlint complains about that, too. This is a bit of a deal breaker. Things should go into %{_libdir} typically.

(In reply to Andy Mender from comment #53)
> Extra Koji build from the latest SRPM:
> https://koji.fedoraproject.org/koji/taskinfo?taskID=55638767
> 
> Fails on s390x, but I don't think it's the SRPMs fault.
> 

Interesting. It's more interesting that the built compiler just doesn't really work at all on armv7.

> > It now carries a patch file, 2 actually, to work around issues with the GDB tests. There are actually lots of failures with the GDB tests, so I limited the suite to gdb.base to make it more workable. There were additional issues with GDB ix86 tests, so that carries an additional patch. I think patching conditionally based on architecture is okay?
> 
> rpmlint has mixed feelings about this, but I think it's okay:
> > msp430-elf-toolchain.src: W: %ifarch-applied-patch Patch1: msp430-elf-toolchain_i386_testfix.patch
> 
> I found this, though:
> https://listman.redhat.com/archives/fedora-devel-list/2007-September/
> msg01515.html
> 

Yup, already stumbled across that particular failure. I also really don't like the idea of an architecture specific patch in general, but it's a little more palatable for a test.

> > Provides: bundled(gnulib)
> 
> Didn't notice this before, but rpmlint complains that this should be
> versioned:
> > msp430-elf-toolchain.src:61: W: unversioned-explicit-provides bundled(gnulib)
> 
> 
> Which does make sense, because without versioning it's impossible to tell
> which gnulib it is. Unless, it's not possible to ascertain, because the
> source is a tarball and not much more.
> 

Okay, looking into this more (also see comment 11, comment 12, comment 13, comment 14), the biggest issue I see is that it should be marked as bundled with gdb and binutils, not gcc. I don't think there's actually a bundled gnulib in gcc. That's probably why I was confused above. Assuming the ChangeLog is correct, I will version it with a date as done in system gdb[0].

> > case $a in
> > # Prevent brp-strip* from trying to handle foreign binaries
> > */brp-strip*)
> >   b=$(basename $a)
> >   sed -e 's,find "*$RPM_BUILD_ROOT"*,find "$RPM_BUILD_ROOT" -not -path "%{buildroot}%{_prefix}/%{target}/lib/gcc/%{target}/%{gcc_version_base}/*" -not -path "%{buildroot}%{_prefix}/%{target}/%{target}/lib/*",' $a > $b
> >   chmod a+x $b
> >   ;;
> > esac
> > done
> 
> Is it possible to use %{buildroot} instead of $RPM_BUILD_ROOT in the sed
> call? Both should expand to the same path.
> 

As mentioned in comment 32, comment 34, I'm not using the variable, I'm replacing the literal string "$RPM_BUILD_ROOT" in the strip  macro.

> rpmlint also complains about quite a lot of header files in the main
> subpackages:
> > msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libgcc.a
> > msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libgcov.a
> > msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libmul_16.a
> > msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libmul_32.a
> > msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libmul_f5.a
> > msp430-elf-gcc.x86_64: W: devel-file-in-non-devel-package /usr/msp430-elf/lib/gcc/msp430-elf/9.2.0/430/exceptions/libmul_none.a
> > [...]
> 
> I'm guessing these are needed by the main packages and the lines can be
> ignored?
> 
> Not sure about these, though:
> > msp430-elf-gcc.x86_64: W: dangling-relative-symlink /usr/lib/.build-id/3d/b4b5215ce578d5b13456988092f30187ac4beb ../../../../usr/msp430-elf/libexec/gcc/msp430-elf/9.2.0/cc1plus
> 
> This symlink doesn't look valid. Is it an artifact from the build?

I'm not entirely sure on that one. Looking into it now.

> 
> > msp430-elf-binutils.x86_64: W: non-standard-dir-in-usr msp430-elf
> 
> > Additionally, unprefixed symlinks to the executables are now made in /usr/msp430-elf/msp430-elf/bin as their presence is expected by the generated msp430-elf-gcc. I don't fully understand why. This eliminates the need for the -B/usr/bin/msp430-elf- compile option, maintaining better compatibility with Makefiles. Again, I'm sure there's a better way but I don't know it.
> 
> Yeah, rpmlint complains about that, too. This is a bit of a deal breaker.
> Things should go into %{_libdir} typically.

I'm not sure I understand. In comment 23, comment 24, comment 25, and comment 35 it was decided to move everything to the /usr/msp430-elf prefix and symlink prefixed binaries as described in the packaging guidelines[1]. Why are we now moving things back to unprefixed system %{_libdir}? These unprefixed binaries should never come into contact with any non-msp430 compiler, and as far as I can tell getting rid of them breaks compatibility with how "upsteam" (TI, so, more midstream...) intends the compiler to work, which as far as I'm concerned would make this package a little pointless.

[0] - https://src.fedoraproject.org/rpms/gdb/blob/master/f/gdb.spec#_105
[1] - https://fedoraproject.org/wiki/Packaging_Cross_Compiling_Toolchains#Cross-compiling_GCC_tool-chains

> Okay, looking into this more (also see comment 11, comment 12, comment 13, comment 14), the biggest issue I see is that it should be marked as bundled with gdb and binutils, not gcc. I don't think there's actually a bundled gnulib in gcc. That's probably why I was confused above. Assuming the ChangeLog is correct, I will version it with a date as done in system gdb[0].

I checked the regular gcc package and yes, it's not there: https://src.fedoraproject.org/rpms/gcc/blob/master/f/gcc.spec
It's bundled with gdb, however: https://src.fedoraproject.org/rpms/gdb/blob/master/f/gdb.spec#_105

> I'm not sure I understand. In comment 23, comment 24, comment 25, and comment 35 it was decided to move everything to the /usr/msp430-elf prefix and symlink prefixed binaries as described in the packaging guidelines[1]. Why are we now moving things back to unprefixed system %{_libdir}? These unprefixed binaries should never come into contact with any non-msp430 compiler, and as far as I can tell getting rid of them breaks compatibility with how "upsteam" (TI, so, more midstream...) intends the compiler to work, which as far as I'm concerned would make this package a little pointless.

Yes, you're right. Apologies for my previous comment. I confused myself. The way it is right now makes more sense, of course. Let's keep that part as it is now.

(In reply to Andy Mender from comment #55)
> > Okay, looking into this more (also see comment 11, comment 12, comment 13, comment 14), the biggest issue I see is that it should be marked as bundled with gdb and binutils, not gcc. I don't think there's actually a bundled gnulib in gcc. That's probably why I was confused above. Assuming the ChangeLog is correct, I will version it with a date as done in system gdb[0].
> 
> I checked the regular gcc package and yes, it's not there:
> https://src.fedoraproject.org/rpms/gcc/blob/master/f/gcc.spec
> It's bundled with gdb, however:
> https://src.fedoraproject.org/rpms/gdb/blob/master/f/gdb.spec#_105
> 

And for some reason this tarball bundles (a different version!) with binutils as well. I'm now versioning both with the last date in the ChangeLog, based on this quote[0] from the packaging guidelines: "A very general rule of thumb is to use the oldest version that seems reasonable as the reason we're doing this is to tell when a library contains issues that have been fixed in newer upstream versions.".

> > I'm not sure I understand. In comment 23, comment 24, comment 25, and comment 35 it was decided to move everything to the /usr/msp430-elf prefix and symlink prefixed binaries as described in the packaging guidelines[1]. Why are we now moving things back to unprefixed system %{_libdir}? These unprefixed binaries should never come into contact with any non-msp430 compiler, and as far as I can tell getting rid of them breaks compatibility with how "upsteam" (TI, so, more midstream...) intends the compiler to work, which as far as I'm concerned would make this package a little pointless.
> 
> Yes, you're right. Apologies for my previous comment. I confused myself. The
> way it is right now makes more sense, of course. Let's keep that part as it
> is now.

Don't worry about it, this has turned into quite the epic.

As for the dangling symlink weirdness from comment 53, it seems to be a bug[1]. cc1plus is excluded from msp430-elf-gcc so it doesn't get packaged. But for some reason the debuginfo symlink makes it into both the msp430-elf-gcc and msp430-elf-gcc-c++ debuginfo packages.

I'm still looking into what's going on with armhfp. I don't fully understand the s390x issue either, but since that's an alternative architecture it's less of a problem, right?

[0] - https://fedoraproject.org/wiki/Bundled_Libraries?rd=Packaging:Bundled_Libraries#Requirement_if_you_bundle
[1] - https://bugzilla.redhat.com/show_bug.cgi?id=878863

> I'm still looking into what's going on with armhfp. I don't fully understand the s390x issue either, but since that's an alternative architecture it's less of a problem, right?

s390x is still listed here as a supported architecture: https://fedoraproject.org/wiki/Architectures
However, I remember reading someplace that it's no longer considered. Also, it's an alternative architecture so not the main focus. Worst case scenario, we can set an ExcludeArch for both s390x and armhfp if the bugs on either of them cannot be easily resolved: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_build_failures

Okay, I can verify the compiler works on armhfp. I'll just disable tests on that platform.

Looking into s390x now, that will be harder since I don't seem to have any IBM big iron laying around...

I cannot recreate s390x failures locally, even tests work fine. I have no idea what the issue is on koji. I experimented some with building on copr but it seems to timeout even with the max possible value[0].

I also poked at the armhfp test failues some more. They're segfaults (in qemu?), not regular test failures. I again have no idea what the issue is. I tried patching out the tests that segfault and then started running into tests that hang indefintely (eg. gdb.base/utf8-identifiers.exp). I think the best course of action is to disable tests for armhfp, and possibly s390x as well to work around timeouts there?

My last remaining question is why I cannot get all packages to install by doing a `dnf install msp430-elf-toolchain` as described in comment 35. I thought the issue was the wrong requires on the main package, but I've corrected those in the latest spec[1] (lines 50-53) and it still doesn't work. Any thoughts?

[0] - https://copr.fedorainfracloud.org/coprs/nielsenb/msp430-development-tools/build/1808709/
[1] - https://download.copr.fedorainfracloud.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01807519-msp430-elf-toolchain/msp430-elf-toolchain.spec

> I cannot recreate s390x failures locally, even tests work fine. I have no idea what the issue is on koji. I experimented some with building on copr but it seems to timeout even with the max possible value[0].

Theoretically, Koji uses the same mock environment you would use locally, but of course resource allocation is different. Same with COPR. In some cases like this one, the errors are not reproducible :(.

> I also poked at the armhfp test failues some more. They're segfaults (in qemu?), not regular test failures. I again have no idea what the issue is. I tried patching out the tests that segfault and then started running into tests that hang indefintely (eg. gdb.base/utf8-identifiers.exp). I think the best course of action is to disable tests for armhfp, and possibly s390x as well to work around timeouts there?

armhfp is actually virtualized on x86_64 machines, at least that's what I see in COPR. So you might run into resource limitations (especially hard disk space) and weird errors again. 

In general I would disable tests altogether if they're the source of issues and the compiler toolchain itself works as intended. If it was possible to create patches applicable to *all* supported architectures to fix the tests, these could then be upstreamed to TI for inclusion in future tarballs. But since upstream is really just the tarballs and errors on Koji and COPR are not reproducible, I'd say leave them out :).

There is also the option to make tests conditional altogether by adding "%bcond_with tests" at the top of the SPEC file and enclosing the test calls in "%if %{with tests} <> %endif", and add a comment explaining why the tests are conditional (f.e. fails in build systems on virtualized hardware). Then, you can run a local build with "rpmbuild -ba --with tests" to run the tests explicitly. More here: http://rpm.org/user_doc/conditional_builds.html

> My last remaining question is why I cannot get all packages to install by doing a `dnf install msp430-elf-toolchain` as described in comment 35. I thought the issue was the wrong requires on the main package, but I've corrected those in the latest spec[1] (lines 50-53) and it still doesn't work. Any thoughts?

I noticed that the last successful armhfp build from COPR mentioned in [0] doesn't generate a msp430-elf-toolchain binary RPM at all. There is also no %files section for msp430-elf-toolchain so the SPEC file now behaves in a way similar to Golang and Python packages - the SRPM is msp430-elf-toolchain, but the generated RPMs are only for the subpackages. I don't know how to make it so that msp430-elf-toolchain registers in dnf as a metapackage, but I found this: https://docs.fedoraproject.org/en-US/Fedora_Contributor_Documentation/1/html/Software_Collections_Guide/sect-Creating_a_Meta_Package.html

Spec URL: https://download.copr.fedorainfracloud.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01822678-msp430-elf-toolchain/msp430-elf-toolchain.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01822678-msp430-elf-toolchain/msp430-elf-toolchain-9.2.0.0-7.fc34.src.rpm

I agree, I've disabled the tests, and stopped trying to generate an installable msp430-elf-toolchain package. I don't think an SCL is a road worth pursuing here. armhfp and s390x builds still seem to timeout[0][1], with s390x still showing inexplicable build failures on F32.

[0] - https://copr.fedorainfracloud.org/coprs/nielsenb/msp430-development-tools/build/1822678/
[1] - https://copr.fedorainfracloud.org/coprs/nielsenb/msp430-development-tools/build/1822883/

Spec URL: https://download.copr.fedorainfracloud.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01945609-msp430-elf-toolchain/msp430-elf-toolchain.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/nielsenb/msp430-development-tools/fedora-rawhide-x86_64/01945609-msp430-elf-toolchain/msp430-elf-toolchain-9.2.0.0-9.fc34.src.rpm

Builds still timeout with armhfp and s390x.

Tests can be be run by building with "--with testsuite". Tests still fail on armhfp.

dwarf4 is forced on Fedora 33 and above as the built gdb seems broken with dwarf5.

Stack protection is disabled on ix86 targets as compile fails with "__stack_chk_fail_local isn't defined" errors otherwise. I'm not sure what changed there.

I'm also not sure why I get a duplicate build id now, fedora-review complains with:

Package does not contain duplicates in %files.
Note: warning: File listed twice: /usr/lib/.build-id/32/8183f7541cf0616ae8b1046b8da9b0b33ee67f

Build 02086380[0] works fine in F34 / Rawhide. No changes to the specfile. armhfp / s390x builds still time out.

[0] - https://copr.fedorainfracloud.org/coprs/nielsenb/msp430-development-tools/build/2086380/