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.

Sign up for GitHub

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

Jump to bottom

Update toolchain with new versions of FPGA tools and remove fw/apps/programmer toolschains #267

Merged
merged 1 commit into from
Oct 17, 2024
Merged

Update toolchain with new versions of FPGA tools and remove fw/apps/programmer toolschains #267

merged 1 commit into from
Oct 17, 2024

Conversation

jthornblad
Copy link
Contributor

@jthornblad jthornblad commented Sep 26, 2024 •
edited by mchack-work
Loading

Description

 - Change docker image to ubuntu 24.04

 - Add new versions of:
        * clang (18.1.3, part of ubuntu 24.04)
        * icestorm (commit 738af822905fdcf0466e9dd784b9ae4b0b34987f)
        * yosys (0.45)
        * nextpnr (0.7) + extra patches for RNG seed handling and early exit
        * iverilog (v12)
        * verilator (v5.028)
        * verible (v0.0-3795)
        * cocotb (v1.9.1)

 - Remove:
        * gcc-arm-none-eabi
        * libnewlib-arm-none-eabi
        * libstdc++-arm-none-eabi-newlib
        * splint
        * pico-sdk
        * golang

Add lint flag needed for new Verilator version.

Type of change

Please tick any that are relevant to this PR and remove any that aren't.

  • Bugfix (non breaking change which resolve an issue)
  • Feature (non breaking change which adds functionality)
  • Breaking Change (a change which would cause existing functionality to not work as expected)
  • Documentation (a change to documentation)

Submission checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my changes
  • I have tested and verified my changes on target
  • My changes are well written and CI is passing
  • I have squashed my work to relevant commits and rebased on main for linear history
  • I have added a "Co-authored-by: x" if several people contributed, either pair programming or by squashing commits from different authors.
  • I have updated the documentation where relevant (readme, dev.tillitis.se etc.)

@jthornblad jthornblad requested review from secworks, agren, dehanj and mchack-work and removed request for secworks September 26, 2024 14:27
@dehanj
Copy link
Member

dehanj commented Sep 30, 2024

Nice!

  • What is the new size?
  • Have you done any testbuilding with the container? For example are all CI jobs in this repo passing with this container (running them locally)?
  • The commit message says that sdcc is removed, but it isn't.

@jthornblad
Copy link
Contributor Author

  • What is the new size?

About 2.5 GB

  • Have you done any testbuilding with the container? For example are all CI jobs in this repo passing with this container (running them locally)?

Yes

  • The commit message says that sdcc is removed, but it isn't.

Updated

@jthornblad jthornblad force-pushed the updated_toolchain branch 3 times, most recently from 1785807 to 7464321 Compare October 9, 2024 09:59
@dehanj
Copy link
Member

dehanj commented Oct 10, 2024

I have tested all make targets in all Makefiles, and all CI jobs.
Also tested the different bitstreams on target.

Found some issues not related to the OCI image, pushed fix to #271

Comments:

  1. I think the commit message should state that clang is updated to version 18.1.3.
    Since we depend on it for reproducible builds.

  2. Splint is removed in the new image. It has not been running in CI, but there is a make target for it, and annotations in firmware. I don't have any strong opinions about specifically splint, AFAIK it is unmaintained.
    There is most likely other we can use, and maybe ones that have GH actions already - then we don't need them in our container. This is all bringing this issue up again Add more (security) code checkers to CI #176.
    Jonas, did you have any more/other tools as suggestion compared to Add more (security) code checkers to CI #176?

@dehanj
Copy link
Member

dehanj commented Oct 10, 2024 •
edited
Loading

Apart from my previous comments, I'm happy.

I still want @mchack-work to take a look, since he has more history with our OCI-image.

@jthornblad
Copy link
Contributor Author

jthornblad commented Oct 11, 2024 •
edited
Loading

  1. I think the commit message should state that clang is updated to version 18.1.3.
    Since we depend on it for reproducible builds.

Done

  1. Splint is removed in the new image. It has not been running in CI, but there is a make target for it, and annotations in firmware. I don't have any strong opinions about specifically splint, AFAIK it is unmaintained.
    There is most likely other we can use, and maybe ones that have GH actions already - then we don't need them in our container. This is all bringing this issue up again Add more (security) code checkers to CI #176.
    Jonas, did you have any more/other tools as suggestion compared to Add more (security) code checkers to CI #176?

We could use scan-build instead, and that is already included from the clang-tools package that is a dependency from clang-tidy.

@mchack-work
Copy link
Member

Relates to #272

@mchack-work mchack-work linked an issue Oct 14, 2024 that may be closed by this pull request
Splitting tkey-builder into multiple OCI images #272
Open
3 tasks
@mchack-work mchack-work removed a link to an issue Oct 14, 2024
Splitting tkey-builder into multiple OCI images #272
Open
3 tasks
@jthornblad jthornblad self-assigned this Oct 14, 2024
mchack-work
mchack-work reviewed Oct 15, 2024
View reviewed changes
Copy link
Member

@mchack-work mchack-work left a comment

Choose a reason for hiding this comment

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

I have:

  • built the OCI image with podman.
  • built bitstream.
  • tested the design on target hardware.

Everything works. Nice work!

There's a false positive, sort of, in the CI. The check-hashes job passes but if one uses the image built from our new Dockerfile the hashes are different. I don't think we should do anything with it in this branch but it's good to be aware of it when we publish the new image in the registry and start using it.

CI fails for the check-verilog job. That's because the verilog linter in tkey-builder:4 is confused by the flags now used. This means CI will always fail until we change to a new tkey-builder version in the CI... Can we postpone the changes to the Makefile until we have published the new tkey-builder?

contrib/Dockerfile Show resolved Hide resolved
contrib/Dockerfile Show resolved Hide resolved
contrib/Dockerfile Show resolved Hide resolved
hw/application_fpga/Makefile Outdated Show resolved Hide resolved
@mchack-work mchack-work mentioned this pull request Oct 15, 2024
Open
3 tasks
@jthornblad @mchack-work
Update toolchain with new versions of FPGA tools and remove fw/apps/p…
f666174 
…rogrammer toolschains

 - Change docker image to ubuntu 24.04

 - Add new versions of:
	* clang (18.1.3, part of ubuntu 24.04)
	* icestorm (commit 738af822905fdcf0466e9dd784b9ae4b0b34987f)
	* yosys (0.45)
	* nextpnr (0.7) + extra patches for RNG seed handling and early exit
	* iverilog (v12)
	* verilator (v5.028)
	* verible (v0.0-3795)
	* cocotb (v1.9.1)
 - Remove:
	* gcc-arm-none-eabi
	* libnewlib-arm-none-eabi
	* libstdc++-arm-none-eabi-newlib
	* pico-sdk
	* golang
@mchack-work mchack-work self-requested a review October 17, 2024 09:28
mchack-work
mchack-work approved these changes Oct 17, 2024
View reviewed changes
Copy link
Member

@mchack-work mchack-work left a comment

Choose a reason for hiding this comment

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

Fixed CI: we apply the lint flag changes to the Makefile later when we change the tkey-builder version used in CI.

Not very happy with code patching, even if it's small, in the Dockerfile but since things are moving so fast I don't see any alternatives either than maintaing a fork of nextpnr of our own. That seems like too much work. We will have to revisit this later, I think.

I think some of the git clones can be replaced by using actual release tar balls sometimes, which might be preferable. We might also want to include digests to see that the downloaded file is what we expected.

I'll open issues for these things but approve for now so we can continue working.

@mchack-work mchack-work merged commit f666174 into main Oct 17, 2024
5 checks passed
@jthornblad jthornblad deleted the updated_toolchain branch October 17, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchack-work mchack-work mchack-work approved these changes

@agren agren Awaiting requested review from agren

@dehanj dehanj Awaiting requested review from dehanj

Assignees

@jthornblad jthornblad

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jthornblad @dehanj @mchack-work