Skip to content

fix Github Action build Firmware#579

Merged
JF002 merged 2 commits intoInfiniTimeOrg:developfrom
mabuch:fix-github-build
Sep 1, 2021
Merged

fix Github Action build Firmware#579
JF002 merged 2 commits intoInfiniTimeOrg:developfrom
mabuch:fix-github-build

Conversation

@mabuch
Copy link
Contributor

@mabuch mabuch commented Aug 14, 2021

This fixes the github Action for Firmware building.
It is largely based on pull request #294 from nanch (which I critically reviewed).

There are a few small differences:

  • Instead of using ACTIONS_ALLOW_UNSECURE_COMMANDS: 'true' for step "Install Embedded Arm Toolchain arm-none-eabi-gcc" instead the most current version (1.0.4) of fiam/arm-none-eabi-gcc is used, this now again works with the github automation (as I find this solution to be cleaner).
  • Using cmake option -DBUILD_DFU=1 to build DFU and therefore removing the obsolete step
  • Removing the unecessary step to build the firmware as this is also included in cmake and changing the unzip step to use the created zip-file.

This is my first pull request ever, in case I should do something different next time, please provide me with feedback.
Thanks

@JF002
Copy link
Collaborator

JF002 commented Aug 14, 2021

Thanks for your PR, @mabuch .

However, there are already a bunch or PRs trying to fix these build issues on Github Actions. I grouped them into the CI project.

As I don't know Actions very well, I'm not sure which PR I should merge and why.

Some time ago, I tried to focus on #333 as it provides added value (summary of memory map changes), but the creator of this PR didn't answer my question (probably because I took too much time to review it) and I forgot about it.

Sooo if you have any opinion on these PR, I'm interested :)

@mabuch
Copy link
Contributor Author

mabuch commented Aug 14, 2021

Hi @JF002
thank you for referring me to the CI project, I will have a look at it today or tomorrow.

At least one suggested improvement mentioned in the comments from PR #333 should be applied and is missing here, but I have not looked at the added function yet (which was the main selling point for #333).
I will try to look through all CI PR and then update mine accordingly, to include all mentioned improvements (should I find them acceptable).

I should mention that I am by no means an expert on Github Actions, this is my first time working with them. But I am willing to dig through documentation and the mentioned reviews.

I will try get back to you in a few days with a consolidated view on all PR regarding the Github Action firmware build.

@Riksu9000
Copy link
Contributor

You shouldn't just copy other peoples changes here, because then we would just have many PRs that do the same thing. You should first help the other PRs by reviewing them.

@mabuch
Copy link
Contributor Author

mabuch commented Aug 14, 2021

@JF002, I was quicker than expected going through the other PR; here are my findings

#145 is fully resolved with this PR; this PR applies the proposed solution from #333 and #534 from @T0RAT0RA which is in my view cleaner than the one from #145 (I did not see the solution from @T0RAT0RA before but upon my research came to the same conclusion).

#413 is a duplicate and solves one of the problem also resolved by #145, #333, #534 and this PR

#414 is mostly also discussed in #333, #534, #294 and this PR. Additionally #414 proposes to also increase the used version of cmake. I have not found any necessity to do so, therefore I have not included it.

#126 proposed other triggers on when to run the Github action (other PR include changes on the triggers as well, but none of the others brings forward a reason why they changed the triggers). I have not touched the triggers, as this is not a technical issue. I do not see any urgent need to change the triggers, therefore I avoided this debate and kept it on the technical aspects (I do not know neither the best practices nor the project-specific desired times when this build should be run). Therefore my PR does not solve this, but it seems that so far only @AirHamster sees this as a problem.


In summary:
My PR solves the broken Github Action without changing the trigger points. It includes the suggestions and solutions from #145, #294, #333 (bugfix parts, not additional functionality), #413, #414, and #534 with the following exceptions: #414 suggests to increase the used cmake version, this can naturally be done but is not necessary to fix the broken build

This PR additionally to previous suggestions also removes some (now) unnecessary steps that are performed by cmake anyhow (I guess the cmake script was updated in the meantime).

I would therefore suggest to go forward with this PR regarding the broken build.

The proposed additional feature of #333 seems nice, but it is additional functionality. Everything needed to make the build run again is taken into consideration with this PR; I find it best to keep the additional functionality separate. I will look at #333 it in the upcoming days and then either add to #333 or send a new PR with the functionality proposed there. It would further complicate things to mix them, therefore I propose to accept this PR, close #145, #294, #413, #414, and #534 as they will be resolved after this and treat #333 as a feature proposition.

@mabuch
Copy link
Contributor Author

mabuch commented Aug 14, 2021

@Riksu9000 yes I understand this, but it there are already quite a lot of duplicates (see my summary above). Additionally there already is a ton of reviews on these original PR, but the authors of the PR never came to update their PR based on the suggestions from the reviews. Therefore I propose to use this PR to resolve the broken build (and only the broken build) and close #145, #294, #413, #414, and #534 as they overlap quite significantly.

However if you think it would be better to close my PR as duplicate and continue working with one or all of the other PR I will be happy to oblige.

@Riksu9000
Copy link
Contributor

Yeah in this case it isn't a big issue as the changes seem very small. I wasn't sure what all needed to be done because I know nothing about Github Actions. This PR seems small, simple and clean and if it fixes all issues then I think its fine.

@JF002
Copy link
Collaborator

JF002 commented Sep 1, 2021

@mabuch Thank you so much for your thorough analysis. I decided to merge your branch to finally make the Github CI work again :) This will allow other contributors to add features or improve them without fixing it again and again.

My apologies to everyone who (tried to) contribute to this CI workflow!

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.

3 participants