Skip to content

CI macOS #867

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

CI macOS #867

wants to merge 2 commits into from

Conversation

urbanij
Copy link

@urbanij urbanij commented Apr 16, 2025

Fixed macOS compilation issue and added to CI

CMakeLists.txt Outdated
@@ -97,7 +97,7 @@ if (DOWNLOAD_GMP)
INTERFACE_INCLUDE_DIRECTORIES "${INSTALL_DIR}/include")
add_dependencies(GMP::GMP gmp)
else()
find_package(GMP)
find_package(GMP REQUIRED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Required is missing here on purpose to provide a nicer error message, please drop this change.

@urbanij
Copy link
Author

urbanij commented Apr 17, 2025

Thanks for the comments, I will fix this later today.

Copy link

github-actions bot commented Apr 17, 2025

Test Results

400 tests  ±0   400 ✅ ±0   1m 53s ⏱️ ±0s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit bbbf159. ± Comparison against base commit 8ff2a70.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. Just a few minor comments.

@@ -18,6 +18,7 @@ target_include_directories(sail_runtime
PUBLIC "${sail_dir}/lib"
)
target_compile_options(sail_runtime PRIVATE -Wno-extra -Wno-unused)
target_link_libraries(sail_runtime PUBLIC GMP::GMP)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think with this here you can remove GMP::GMP from here:

    target_link_libraries(riscv_sim_${arch}
        PRIVATE softfloat sail_runtime default_config GMP::GMP
    )

if: startsWith(matrix.os, 'macos-') && steps.opam-cache.outputs.cache-hit != 'true'
run: |
opam init --auto-setup --bare
opam switch create default 4.14.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably could use OCaml 5 here?

opam switch create default 4.14.1
eval $(opam env)
opam update
opam install -y sail
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should lock the version (don't ask me how!).

Copy link
Collaborator

Choose a reason for hiding this comment

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

opam install -y sail.0.19 should work.

@urbanij urbanij force-pushed the furbani/ci-macos branch 3 times, most recently from 20cb907 to f435677 Compare April 26, 2025 14:00
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