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

ci: Use up to date actions, build against multiple ghidra versions. #5

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

antoniovazquezblanco
Copy link
Contributor

Checklist

  • Closing issues: None
  • Mark this if you consider it ready to merge
  • I've added tests (optional)
  • I wrote some documentation

Description

Minor CI improvements. Mostly, you were using an obsolete github action. I happen to maintain a similar action because the previous one was abandoned.

I also wanted to try and provide builds for different ghidra versions because Ghidra is sometimes picky with installing plugins for other versions. Please, feel free to ask me to add or remove other versions.

To be done
If you want to further improve this I would recommend to always build (for pr testing) and only release on tag creation but I did not want to change your "release" policy without you confirming if you really want this. Please, let me know if you want me to do that.

Thanks!

@antoniovazquezblanco
Copy link
Contributor Author

Also, if you find that you are using a lot the plugin and the API usage hits rate-limiting it is possible to add a token to avoid the problem. :)

@v-p-b
Copy link
Collaborator

v-p-b commented Jan 6, 2025

Thanks for the PR! A few questions (I'm new to GH Actions, sry):

  • Shouldn't the Ghidra version be dynamically referenced in the Setup Ghidra step?
  • What's the benefit of creating an artifact first for releases?

Re: releases, since the project is in a very early stage my original idea was to manually tag commits (as required for releases) as appropriate and run the workflow manually to create releases. I think this can be done with your changes too?

@antoniovazquezblanco
Copy link
Contributor Author

Shouldn't the Ghidra version be dynamically referenced in the Setup Ghidra step?

Oops! My fault! You are totally right. Fixed now.

@antoniovazquezblanco
Copy link
Contributor Author

What's the benefit of creating an artifact first for releases?

Rather than a benefit is more kind of a "need". Let me explain:

If you want to build against different ghidra versions you need to setup multiple pipelines with multiple versions of ghidra. This is acomplished via the matrix option in the pipeline.

When this happens, "n" pipelines will be run. Ideally, you would want for all of them to be run successfully before cuttting a release. If one or more failed you would want to decide what to do with that particular version. This is acomplished by the "requires" tag.

This kind of sets the first reason to split the "build" and "release" phases of the pipeline. There may be others such as maybe including a testing phase in between or simply to keep things a little bit more compartimentalized... Different reasons for everyone...

Once you reach to the point that separating those build steps may be beneficial, the only way to exchange files between them is to use artifacts.

Finally, with this structures, multiple other side benefits arise:

  • You may always run the "build" step, not only in branch main. This allows you to test against multiple ghidra versions everytime you push or on all the pull requests. Basic test for everything.
  • You may download the resulting build artifacts for a particular amount of time without having to cut a release. This is useful when a test fails because it may allow you to get a compiled binary without having to have all ghidra versions in your local PC...
  • You may have a different policy to run the release action. This means you can decide wether to cut a release based on any action you may like. Manual triggering, tag pushing, etc.

I hope this helps to clarify the pipeline :D

@v-p-b
Copy link
Collaborator

v-p-b commented Jan 6, 2025

Thank you for the fix and the explanation! I'll merge this.

@v-p-b v-p-b merged commit 5ce454f into radareorg:master Jan 6, 2025
10 checks passed
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.

2 participants