Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add TI msp430 gcc 8.3.0 ELF/newlib toolchain #91

Merged
merged 1 commit into from
Oct 19, 2019

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Oct 15, 2019

This PR adds the current official TI msp430 ELF toolchain.

@gschorcht @geromueller please take a look!

Compared to #82 , it avoids building the toolchain from source.
Compared to #62, this is using TI's current release, and does not install the support files.

PR to RIOT master updating msp430: RIOT-OS/RIOT#12457, which includes just the used msp430-support-files.

This PR will not do any compilation test on travis, as there's no code supporting this in master...

@geromueller
Copy link

FYI: A couple of weeks ago I asked the riot-devs per email which version to use, the TI or vanilla one. The vote was leaning towards the vanilla version.

@kaspar030
Copy link
Contributor Author

FYI: A couple of weeks ago I asked the riot-devs per email which version to use, the TI or vanilla one. The vote was leaning towards the vanilla version.

I asked the same, and the vote was more like, "why maintain our own toolchain compiles?". I was leaning towards vanilla, as TI's up to 8.2.0 needed patches to their own linker files.
But the new version works just fine.

Are there any concrete reasons for using the vanilla version?

@gschorcht
Copy link
Contributor

It seems that the installation of the Ubuntu packages could be removed, right?

riotdocker/Dockerfile

Lines 90 to 93 in c6e22b8

&& echo 'Installing MSP430 toolchain' >&2 && \
apt-get -y --no-install-recommends install \
gcc-msp430 \
msp430-libc \

@geromueller
Copy link

Maybe

  • Use different compiler flags for newlib
  • Use and test any gcc version, even the latest

@kaspar030
Copy link
Contributor Author

It seems that the installation of the Ubuntu packages could be removed, right?

Yeah, for now we'll need to keep this, as current master relies on it. For a while (until the release after RIOT-OS/RIOT#12457), we'll have to keep both.

@gschorcht
Copy link
Contributor

gschorcht commented Oct 16, 2019

I will take a look to PR RIOT-OS/RIOT#12457 next, as soon as I have some time. At the beginning of the winter term, my schedule is always quite busy.

@gschorcht
Copy link
Contributor

I was looking for the votes you both mentioned here.

@geromueller You had exactly two votes from other developers, but both of them were suggesting to use vanilla. Arguments were:

  • More consistent to other toolchains (no TI specific stuff)
  • Does not rely on TI for future updates (in case TI looses interest in maintaining their on version)

@kaspar030 I couldn't find your survey.

From my humble point of view, we shouldn't compile a toolchain by our self if the precompiled one from the vendor or any other source works and fulfills our requirements. It is not always necessary to have the latest gcc version. Compiling the toolchain takes a lot of time and resources to generate the Docker image. If I remember correctly, it may timeout during generation, but I'm not sure. And finally, if the vendor doesn't have interest anymore, then we can compile it by our self.

So, if version 8.3.0 works, fulfills our requirements and doesn't have the problems @kaspar030 mentioned for version 8.2.0, I would prefer to use the precompiled one.

I have tested it with RIOT-OS/RIOT#12457 and it seems to work, at least the compilation. I will have a test whether produced code also works. The only thing I have found with the toolchain is that it obviously uses a newlib-nano (tests/libc_newlib fails) which requires to use module newlib-nano in the build system. But this can be fixed in RIOT-OS/RIOT#12457.

@geromueller
Copy link

geromueller commented Oct 18, 2019 via email

@gschorcht
Copy link
Contributor

gschorcht commented Oct 18, 2019

using the TI one as default is perfectly fine for me. Nevertheless there may be occasions when the vanilla one is interesting too. We could use docker ARGS to be able to switch to the vanilla one if a developer want to compile it on his own?

Would gcc 9.2 require further adaptations in RIOT's source code for MSP430?

@gschorcht
Copy link
Contributor

We could use docker ARGS to be able to switch to the vanilla one if a developer want to compile it on his own?

On the other hand, we might argue that the riotdocker image comes with the default toolchain version for developers who do not want to have effort to manually install a toolchain on their host and wanna to use the toolchain out of the box. Developers with very specific toolchain requirements would probably install the toolchain on their host anyway.

@geromueller
Copy link

geromueller commented Oct 18, 2019 via email

@kaspar030
Copy link
Contributor Author

Would gcc 9.2 require further adaptations in RIOT's source code for MSP430?

AFAIK, no. I'm using a self-compiled msp430 (very similar to how @geromueller did it: started from the arch AUR build files, changed some newlib flags). It works fine.

@kaspar030
Copy link
Contributor Author

I'm actually all for compiling our own toolchains (even if just for verifying the vendor ones). But IMO that needs to be introduced properly.

  1. We're relying on vendor provided toolchains for all the other architectures. TI used to have a bad toolchain story, but they have caught up (by outsourcing) to a usable state. IMO, unless there's a reason ("we need compile flag X") and if TI's binaries work, we should use them

  2. if we introduce this here in the docker container, there's basically no way for people to get the same toolchain compile. It'll be spread over the docker container. I could not download "the RIOT authorative msp430 toolchain version X" other than the newest one (by pulling the container and extracting it from there, or by building within the container).

  3. doing the compilation within Docker makes the container build times go up a lot. Even cmake increased the build times (on Docker hub's public cloud and on travis) by minutes. A whole toolchain compile (bootstrap, first stage, second stage...) will be more. Everything twice, as the Murdock build container is built right after this one.

So I propose:

  1. (assuming that TI's toolchain does work), go with the TI toolchain for now, make sure our msp430 port works with it
  2. start a repository for toolchains (RIOT-OS/toolchains)
  3. provide toolchain building scripts there (for all architectures, make them build as similar as possible)
  4. make CI build those toolchains and make them available for download
  5. gauge differences to the official ones
  6. decide if we prefer to use our own binaries as the "official" ones

@kaspar030
Copy link
Contributor Author

We could use docker ARGS to be able to switch to the vanilla one if a developer want to compile it on his own?

The whole point of the docker container is to bundle a somewhat stable toolchain.
If you are using a different one, PATH=<path-to-your-toolchain/bin>:$PATH make ... should be all you need, or am I missing something?

@gschorcht
Copy link
Contributor

gschorcht commented Oct 18, 2019

So I propose:

  1. (assuming that TI's toolchain does work), go with the TI toolchain for now, make sure our msp430 port works with it
  2. start a repository for toolchains (RIOT-OS/toolchains)
  3. provide toolchain building scripts there (for all architectures, make them build as similar as possible)
  4. make CI build those toolchains and make them available for download
  5. gauge differences to the official ones
  6. decide if we prefer to use our own binaries as the "official" ones

I completely agree with @kaspar030.

@gschorcht
Copy link
Contributor

gschorcht commented Oct 18, 2019

From my point of view, we should provide the riotdocker image for only one toolchain version for which we know that it is working with RIOT-OS. We should always use precompiled archives, our own or the official ones. The dockerfile in #82 seems to be much more complex than what we have for all other platforms and it will take a lot of time to generate the image.

Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The toolchain was tested completely together with current master and RIOT-OS/RIOT#12457. It is working without any problems.

@gschorcht
Copy link
Contributor

To make some progress with the new toolchain and RIOT-OS/RIOT#12457, I will merge this first. Regarding the PR #82, we will find a solution according to @kaspar030's proposal in #91 (comment).

@gschorcht
Copy link
Contributor

@kaspar030 Would you like to rebase it before the merge?

@gschorcht
Copy link
Contributor

@kaspar30 Could you rebase your branch to the current master and force it? I have accidentally let GitHub merge the master branch into your branch 🕶️

@kaspar030 kaspar030 force-pushed the add_ti_msp430_elf_toolchain branch from ffb7c28 to 779eb39 Compare October 19, 2019 12:11
@kaspar030
Copy link
Contributor Author

Could you rebase your branch to the current master and force it?

yup, done.

@gschorcht gschorcht merged commit 2945168 into RIOT-OS:master Oct 19, 2019
@gschorcht
Copy link
Contributor

@kaspar030 Thanks for contributing it. Once it is rolled out to Murdock, we can continue with PR RIOT-OS/RIOT#12457.

@gschorcht
Copy link
Contributor

So I propose:

  1. (assuming that TI's toolchain does work), go with the TI toolchain for now, make sure our msp430 port works with it
  2. start a repository for toolchains (RIOT-OS/toolchains)
  3. provide toolchain building scripts there (for all architectures, make them build as similar as possible)
  4. make CI build those toolchains and make them available for download
  5. gauge differences to the official ones
  6. decide if we prefer to use our own binaries as the "official" ones

This might be related to issue #64 which proposes the versioning of riotdocker containers for releases. This would become easy with a toolchain repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants