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

Use arm-none-eabi-gcc-action #6532

Merged
merged 1 commit into from
Jul 4, 2022
Merged

Conversation

jepler
Copy link
Member

@jepler jepler commented Jun 28, 2022

@tannewt noticed there's an action to download the embedded arm compiler. it might even be faster, as it uses the actions cache to store the compiler.

@hathach
Copy link
Member

hathach commented Jun 29, 2022

I have made similar PR to tinyuf2 and nrf52 bootloader repo

It works like a charm, and tidy up yml file as well :)

@dhalbert
Copy link
Collaborator

There are multiple instances of a cache conflict going on in the first set of builds:

Run carlosperate/arm-none-eabi-gcc-action@v1
  with:
    release: 10-2020-q4
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.10.5/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.10.5/x64/lib
Cache miss, downloading GCC 10-2020-q4 from https://developer.arm.com/-/media/Files/downloads/gnu-rm/10-2020q4/gcc-arm-none-eabi-10-2020-q4-major-x86_64-linux.tar.bz2 ; MD5 8312c4c91799885f222f663fc81f9a31
GCC release downloaded, calculating MD5...
Downloaded file MD5: 8312c4c91799885f222f663fc81f9a31
Extracting /home/runner/work/_temp/fd276775-f7ae-4f26-809d-c9313201ec2a
/usr/bin/tar xj --warning=no-unknown-keyword --overwrite -C /home/runner/gcc-arm-none-eabi-10.2020.4-linux -f /home/runner/work/_temp/fd276775-f7ae-4f26-809d-c9313201ec2a
Adding to cache: /home/runner/gcc-arm-none-eabi-10.2020.4-linux
Warning: ⚠️ Could not save to the cache.
Unable to reserve cache with key gcc-arm-none-eabi-10.2020.4-linux, another job may be creating this cache.
Adding /home/runner/gcc-arm-none-eabi-10.2020.4-linux/gcc-arm-none-eabi-10-2020-q4-major/bin to PATH.

@hathach
Copy link
Member

hathach commented Jun 29, 2022

There are multiple instances of a cache conflict going on in the first set of builds:

Run carlosperate/arm-none-eabi-gcc-action@v1
  with:
    release: 10-2020-q4
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.10.5/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.10.5/x64/lib
Cache miss, downloading GCC 10-2020-q4 from https://developer.arm.com/-/media/Files/downloads/gnu-rm/10-2020q4/gcc-arm-none-eabi-10-2020-q4-major-x86_64-linux.tar.bz2 ; MD5 8312c4c91799885f222f663fc81f9a31
GCC release downloaded, calculating MD5...
Downloaded file MD5: 8312c4c91799885f222f663fc81f9a31
Extracting /home/runner/work/_temp/fd276775-f7ae-4f26-809d-c9313201ec2a
/usr/bin/tar xj --warning=no-unknown-keyword --overwrite -C /home/runner/gcc-arm-none-eabi-10.2020.4-linux -f /home/runner/work/_temp/fd276775-f7ae-4f26-809d-c9313201ec2a
Adding to cache: /home/runner/gcc-arm-none-eabi-10.2020.4-linux
Warning: ⚠️ Could not save to the cache.
Unable to reserve cache with key gcc-arm-none-eabi-10.2020.4-linux, another job may be creating this cache.
Adding /home/runner/gcc-arm-none-eabi-10.2020.4-linux/gcc-arm-none-eabi-10-2020-q4-major/bin to PATH.

This is rather normal for initial run, when each of the matrix instance try to add toolchain to cache (only first one succeeded to cache). It will go away afterwards, since toolchain already cached. You could simply re-run the ci, it this warnings will disappear.

@tannewt
Copy link
Member

tannewt commented Jun 29, 2022

You could have the test job do it too since it runs before the builds.

@dhalbert
Copy link
Collaborator

You could have the test job do it too since it runs before the builds.

As long as it's then part of the cache, that sounds great. I think we have like 60 jobs trying to fetch it at the same time, and all are refetching because of the collision.

@jepler
Copy link
Member Author

jepler commented Jun 29, 2022

As long as we don't care about adding an unnecessary ~6s to the 'test job' whether it's needed or not ...

I think that what @hathach is trying to say is, while the message looks troubling due to its unfamiliarity it doesn't indicate a problem; and once the PR has been run on main once and the cache is populated, the message shouldn't occur anymore anyway. Until we cleared the cache or selected a different gcc version that would use a different cache key.

@jepler
Copy link
Member Author

jepler commented Jun 29, 2022

That said, googling about the message led me to concerning problems that DO show this message and where the suggested remedy is to change the cache key: actions/cache#485 (comment) -- problem being, while we can and have done that with esp-idf in the past, there's no provision that I saw to make this action use a different key. hm.

@dhalbert
Copy link
Collaborator

I'm not thinking the message indicates an error, but it's an inefficiency. The first 60 or so jobs are not benefiting from the cache, it would appear.

This is a specialized action, which downloads specific files from specific places. Is there a more generalized action or mechanism we could use to download particular files (in the test job or some other precursor job), that would allow us to clear the cache or change the key if necesary? I haven't yet tried to understand what the espressif jobs do in terms of caching.

@jepler
Copy link
Member Author

jepler commented Jun 30, 2022

.. but only the first time it runs on the main branch. On the next run, all the builds will see the cache and use it.

@dhalbert
Copy link
Collaborator

.. but only the first time it runs on the main branch. On the next run, all the builds will see the cache and use it.

oooh! I thought it was only per PR run.

@dhalbert
Copy link
Collaborator

That said, googling about the message led me to concerning problems that DO show this message and where the suggested remedy is to change the cache key: actions/cache#485 (comment) -- problem being, while we can and have done that with esp-idf in the past, there's no provision that I saw to make this action use a different key. hm.

I would approve this but still a bit worried about getting stuck due to this.

@hathach
Copy link
Member

hathach commented Jun 30, 2022

maybe the author of the action can explain this better. @carlosperate I hope you have an minute or two to clarify the cache warnings, If not sorry for the mention

@carlosperate
Copy link

carlosperate commented Jul 4, 2022

Hi all, and thanks for the tag @hathach!

The action will first try to get GCC from the cache, and if that fails (which would be expected the first time it runs), it will download it from the Arm servers, set it up, and only then try to save it to the cache.
The avaiable cache size is limited, so rather than have a cache entry for each individual job, there is a cache entry per each version/platform of GCC used.

In this case as several of the parallel jobs are attempting to save to the same cache entry at the same time, so we get the warnings discussed here. These are raise as warnings, but the action has already set up GCC and the rest of the job should continue as normal.
Then, as long as at least one of the jobs has correctly saved the toolchain to the cache (which the first one should have done, as the other are blocked by it), all future jobs should be able to fetch them from there. And as far as I am aware, there should not be any issues with all the jobs retrieving data from the same cache entry at the same time.

As a simple test, I've run this in my fork:

I'm not thinking the message indicates an error, but it's an inefficiency. The first 60 or so jobs are not benefiting from the cache, it would appear.

I haven't look at all of the jobs, as there are quite a few, but I've looked at a couple of random ones, and at some point the first run here is already finding matches in the cache, like this one: https://github.com/adafruit/circuitpython/runs/7103081116?check_suite_focus=true#step:5:12
But yes, the first run will see quite a few cache misses for all the jobs that started before the first one had a chance to save the toolchain.

Hope that clarifies things, let me know if I left something out

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks!

@dhalbert dhalbert merged commit 724bea7 into adafruit:main Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants