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

Allow build on MacOS (MX) #30

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Allow build on MacOS (MX) #30

wants to merge 4 commits into from

Conversation

DarkaMaul
Copy link
Collaborator

This PR allows to build quokka on MacOS.
We also update the FindIdaSdk cmake module from BinExport so we support also other IDA variants.

This will now both compile Quokka in 32 bits and 64 bits, closing outstanding issues (#24).

This has been tested on a MacOS M2 computer (and it works).

This PR allows to build quokka on MacOS.
We also update the FindIdaSdk cmake module from BinExport so we support also other IDA variants.

This will now both compile Quokka in 32 bits and 64 bits, closing outstanding issues.

This has been tested on a MacOS M2 computer (and it works).
	# Please enter the commit message for your changes. Lines starting
@DarkaMaul DarkaMaul force-pushed the build/mac-os branch 3 times, most recently from 53e7a5d to 5264981 Compare April 4, 2024 14:24
Copy link
Collaborator

@patacca patacca left a comment

Choose a reason for hiding this comment

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

Good job! I just have added a few remarks, mostly because I think it's better if we don't pin specific minor versions of github actions so we can benefit from the latest versions without having to manually update them.

Nice refactoring on the cmake part as well!


runs-on: ${{ matrix.os }}
steps:
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4.1.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
uses: actions/checkout@v4.1.1
uses: actions/checkout@v4

Unless there are specific reasons to pin the minor version I think it's better to just use the latest v4 so that we don't need to constantly update it


- name: Setup cmake
uses: jwlawson/actions-setup-cmake@v1.12
uses: jwlawson/actions-setup-cmake@959f1116cf9f1ae42fff8ec1a4aaae6d4a0e348b #v2.0.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
uses: jwlawson/actions-setup-cmake@959f1116cf9f1ae42fff8ec1a4aaae6d4a0e348b #v2.0.1
uses: jwlawson/actions-setup-cmake@v2

Same as above

# We need one action per file
# See https://github.com/actions/upload-artifact/issues/331
- name: Upload Artifacts (64)
uses: actions/[email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
uses: actions/upload-artifact@v4.3.1
uses: actions/upload-artifact@v4

with:
name: idaplugin-${{ matrix.os }}-${{ matrix.ida_sdk }}
path: ${{ matrix.ida_sdk }}-quokka_plugin0064.${{ matrix.ext }}
if-no-files-found: error

- name: Upload Artifacts (32)
uses: actions/[email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
uses: actions/upload-artifact@v4.3.1
uses: actions/upload-artifact@v4

steps:
- name: Download Artefact
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4.1.4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
uses: actions/download-artifact@v4.1.4
uses: actions/download-artifact@v4


- name: Release
uses: softprops/action-gh-release@v0.1.14
uses: softprops/action-gh-release@9d7c94cfd0a1f3ed45544c887983e9fa900f0564 #v2.0.4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
uses: softprops/action-gh-release@9d7c94cfd0a1f3ed45544c887983e9fa900f0564 #v2.0.4
uses: softprops/action-gh-release@v2

@@ -1,18 +1,18 @@
include_guard()

get_filename_component(hw_proto "quokka.proto" ABSOLUTE)
# get_filename_component(hw_proto "quokka.proto" ABSOLUTE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# get_filename_component(hw_proto "quokka.proto" ABSOLUTE)

We can just remove the line if not needed

TARGET quokka_proto
LANGUAGE python
OUT_VARS PROTO_PY)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
OUT_VARS PROTO_PY)

I think that PROTO_PY was used only as a temporary var for compiling the protobuf file, with this refactor it shouldn't be needed anymore

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