GhidraBoy icon indicating copy to clipboard operation
GhidraBoy copied to clipboard

CI improvements

Open antoniovazquezblanco opened this issue 1 year ago • 3 comments

antoniovazquezblanco avatar Jan 29 '24 12:01 antoniovazquezblanco

Any feedback on this?

antoniovazquezblanco avatar May 05 '24 18:05 antoniovazquezblanco

Thanks for the pull request! :smile: Unfortunately my focus has been on other projects so I haven't had the time to think about GhidraBoy much recently...

The idea to build for several Ghidra version is great! I guess the main downside is that when a new Ghidra version is released, I need to publish a new GhidraBoy version but that release will then get built binaries for all Ghidra versions, even when there are no other changes. In other words, it creates an unnecessary GhidraBoy release for all the older Ghidra versions. But it's not really a problem, and I guess I can manually drop some of the artifacts from a release if necessary so old Ghidra versions won't be affected.

I'm not so sure about the other CI changes, because you've replaced a simple 10-line script with an external dependency (150+ lines of code) without any "sales pitch" or explanation why I should use your setup-ghidra project. Right now from my point of view you've effectively added 140 lines of code and a dependency that doesn't seem to add extra value. In fact, your project doesn't support everything the 10-line script did, so you've removed a feature: Ghidra release SHA-256 checking.

Gekkio avatar May 11 '24 08:05 Gekkio

Thanks for the feedback. As you can see I added an issue with your SHA checking feature. Unfortunatelly, I am not that good at managing time and will see wen will that be added...

I believe your script suits your needs and that was the way I used to do it that way. I decided to revamp an old setup ghidra action because this is platform independent, includes testing and it was a way to externalize the maintenance of this CI step that was not the focus of my ghidra plugin repos. It is just a convenience for me. I decided to share it with the people that also develops plugins for ghidra that I have used at some point to see if they would be useful to others and to learn about their way to deploy this projects.

Regarding the issue of having to delete old artifacts, I solved that by using another action (see https://github.com/antoniovazquezblanco/GhidraDeviceTreeBlob/blob/main/.github/workflows/main.yml) but I wanted to touch your CI as little as possible.

Your feedback is of great use, I will try to improve setup-ghidra and feel free to use this changes or close the PR following your needs.

Thanks!

antoniovazquezblanco avatar May 13 '24 08:05 antoniovazquezblanco

I've cherry-picked the changes (while keeping original commit author info!) and just added another commit so that the old install script is used. Thanks again! :+1:

And please don't take my comments as serious criticism about setup-ghidra! I'm 100% sure many people prefer to use a simple-to-use third party action like it, and it's just my own preference to avoid dependencies unless they provide a large amount of value.

Gekkio avatar Jun 09 '24 08:06 Gekkio

No worries @Gekkio. Don't take it wrong but the criticism is more than valid and very useful to me. I've added signature checking to the TODO list. It is a feature I missed.

I totally get the not wanting to depend on a dependency unless I can avoid it mindset.

Thanks for the awesome project again. :)

antoniovazquezblanco avatar Jun 10 '24 08:06 antoniovazquezblanco

I have finally found a minute to add this. It is available in the v2.0.0 release. I guess you prefer to stick to the simple script but if you change your mind I will gladly create a PR.

Thanks again for the awesome feedback! :D

antoniovazquezblanco avatar Jun 17 '24 07:06 antoniovazquezblanco