Update macos and ubuntu ci to be more extensive

mcbarton commented 6 months ago

Added section that will run tests on arm osx once free tier github runner available (currently commented out) Build cppyy and tests on macos as well as linux, and where CppInterOp uses clang-repl as well as cling Use homebrew provided clang rather than apple provided clang Test for both clang-repl 16 and 17 for both macos and linux Update ubuntu runner to be consistent across all Ubuntu runners

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (cf18068) 72.75% compared to head (2d93347) 77.55%.

Additional details and impacted files

[![Impacted file tree graph](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/173/graphs/tree.svg?width=650&height=150&src=pr&token=7UWTYSVVT5&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research)](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research) ```diff @@ Coverage Diff @@ ## main #173 +/- ## ========================================== + Coverage 72.75% 77.55% +4.79% ========================================== Files 8 8 Lines 2885 2887 +2 ========================================== + Hits 2099 2239 +140 + Misses 786 648 -138 ``` | [Files](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research) | Coverage Δ | | |---|---|---| | [lib/Interpreter/CppInterOpInterpreter.h](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0NwcEludGVyT3BJbnRlcnByZXRlci5o) | `92.77% <100.00%> (+17.77%)` | :arrow_up: | ... and [5 files with indirect coverage changes](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/173/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research) | [Files](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research) | Coverage Δ | | |---|---|---| | [lib/Interpreter/CppInterOpInterpreter.h](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0NwcEludGVyT3BJbnRlcnByZXRlci5o) | `92.77% <100.00%> (+17.77%)` | :arrow_up: | ... and [5 files with indirect coverage changes](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/173/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research)

mcbarton commented 6 months ago

Issues that have shown as a result of the CI running is ubu22-gcc12-clang-repl-17 fails, but ubu22-gcc12-clang-repl-17-cppyy fails (not entirely sure why, and will look into)

cling based ci runs fails. Unsure why. Only noticed difference from quick glance is that previous run of ci was using cached llvm. This run built from scratch. Any details on cached cling and llvm-project it uses would be useful.

@alexander-penev @vgvassilev I thought from this readme that there was a smaller m1 github runner https://github.com/actions/runner-images/blob/main/images/macos/macos-13-arm64-Readme.md . Given it keeps waiting for runner then it may not exist. Based on this thread then the only github m1 runners may currently be the xl variants https://github.com/actions/runner-images/issues/8439 which cost money. I can change it to this runner if the project is willing to bare the cost until there is a free one. Let me know what you want me to do

It's also worth noting that my ci update was to use macos version 13, but come January there are plans for a beta which uses macos version 14 (latest version). see https://github.com/actions/runner-images/issues/7508

mcbarton commented 6 months ago

More details on the larger macos runners if you decide to proceed with that route for now https://docs.github.com/en/actions/using-github-hosted-runners/about-larger-runners/about-larger-runners . I know from issue https://github.com/compiler-research/CppInterOp/issues/138 that it wasn't expected to be necessary due to llvm caching.

mcbarton commented 6 months ago

By fixing the version of cling in CI to the latest version, rather than using the latest commit I have now been able to have the ci pass for cling based jobs. It is currently hardcoded, but it future the version can be chosen against the version of clang cling is being built alongside.

ubu22-gcc12-clang-repl-17 failing doesn't appear to have anything to do with the changes I have made for this PR.

Now this PR is just waiting on a decision as to what arm based macos runner should it be run on (i.e. the xlarge ones which currently cost money to run on, or wait and see if a free variant is made available)

vgvassilev commented 6 months ago

The cache system requires one successful build of llvm and then the rest is built on top. Could we employ that to avoid the costs of M1?

mcbarton commented 6 months ago

The cache system requires one successful build of llvm and then the rest is built on top. Could we employ that to avoid the costs of M1?

I don't think you can completely avoid the costs of the M1, but you can certainly limit the costs, by using your cache (so run time minutes are low for most runs, and only high if cache needs updating), and have it only run the macos tests by manually activating them instead of automatically like currently. For example you only run it once you are satisfied that the ubuntu tests are all ok for a PR.

mcbarton commented 6 months ago

The only time I can think of that a new llvm cache would need creating for the M1 ci runs is when a new patch is added to the project, so that the ci can test with the new patch.

mcbarton commented 6 months ago

@vgvassilev I managed to find this message https://github.com/actions/runner-images/issues/8439#issuecomment-1755601587 which is relevant. Github plan to offer a free tier for M1 sometime next year (it says the date is to be decided, but they are still ramping up their hardware acquisition until about March 2024). My suggestion is I update this PR to have extensive tests for macos x86, which should catch any ci issues with macos builds. I would comment out the arm based jobs for now until the date for the free tier of m1 runners has been announced, and the correct runner key can be added. I'd update the commit message and PR name accordingly. What do you think?

mcbarton commented 6 months ago

@vgvassilev I managed to find this message actions/runner-images#8439 (comment) which is relevant. Github plan to offer a free tier for M1 sometime next year (it says the date is to be decided, but they are still ramping up their hardware acquisition until about March 2024). My suggestion is I update this PR to have extensive tests for macos x86, which should catch any ci issues with macos builds. I would comment out the arm based jobs for now until the date for the free tier of m1 runners has been announced, and the correct runner key can be added. I'd update the commit message and PR name accordingly. What do you think?

Implementing this suggestion has raised the following issues with ci in this PR

Forgot to change the following cppyy on this line to on https://github.com/mcbarton/CppInterOp/blob/ec4ac34dd8fcf707e845175eea866e5012828e00/.github/workflows/ci.yml#L114
Similarly need to change cppyy on this to on https://github.com/mcbarton/CppInterOp/blob/ec4ac34dd8fcf707e845175eea866e5012828e00/.github/workflows/ci.yml#L159C24-L159C24
Issue with Xcode is stopping the following line passing without error https://github.com/mcbarton/CppInterOp/blob/ec4ac34dd8fcf707e845175eea866e5012828e00/.github/workflows/ci.yml#L454

Once I determine how to solve the xcode related issue I will push a commit solving these issues

mcbarton commented 6 months ago

@alexander-penev I consider this PR ready for review. The issue which stops osx-x86-clang-clang-repl-17-cppyy and osx-x86-clang-clang-repl-16-cppyy passing although appears to be to do with the xcode on the runner, it doesn't appear to be due to this from further inspection, since it passes when CppInterOp uses cling instead of clang-repl. I plan to remove the line sudo xcode-select -s /Applications/Xcode_15.1.app/Contents/Developer from the ci once any issues you raise are fixed, since this xcode isn't available yet despite being in the readme.

github-actions[bot] commented 6 months ago

clang-tidy review says "All clean, LGTM! :+1:"

github-actions[bot] commented 6 months ago

clang-tidy review says "All clean, LGTM! :+1:"

mcbarton commented 6 months ago

@vgvassilev thanks for the suggestion of what was stopping the clang repl cppyy ci jobs from passing, and @alexander-penev for implementing this fix. Do the commits just need to be squashed now, or is this PR in need of further review?

github-actions[bot] commented 6 months ago

clang-tidy review says "All clean, LGTM! :+1:"

github-actions[bot] commented 6 months ago

clang-tidy review says "All clean, LGTM! :+1:"

github-actions[bot] commented 6 months ago

clang-tidy review says "All clean, LGTM! :+1:"

mcbarton commented 6 months ago

@alexander-penev I just tried squashing the commits on my local machine, but it seems to remove you as a contributor when I do so. Can you tell me how I do this while keeping all the contributors in the squashed commit?

kgantchev commented 5 months ago

. I would comment out the arm based jobs for now until the date for the free tier of m1 runners has been announced, and the correct runner key can be added. I'd update the commit message and PR name accordingly. What do you think?

Might I make a suggestion here? Feel free to use FlyCI's M1 and M2 runners. Our runners are on average 2x faster and 2x cheaper than GitHub's AND we have a free tier for OSS projects (see below).

Install Instructrions

Easily replace your M1 runners:

jobs:
 ci:
-    runs-on: macos-latest
+    runs-on: flyci-macos-large-latest-m1
   steps:
   - name: 👀 Checkout repo
     uses: actions/checkout@v4

Or try the M2 runners:

jobs:
  ci:
-    runs-on: macos-latest
+    runs-on: flyci-macos-large-latest-m2
    steps:
      - name: 👀 Checkout repo
        uses: actions/checkout@v4

Pricing

Processor	vCPU	RAM (GB)	Storage	Label	Price on FlyCI	Price on GitHub
M1	4	7	28 GB	flyci-macos-large-latest-m1	$0.06	-
M1	8	14	28 GB	flyci-macos-xlarge-latest-m1	$0.12	$0.16
M2	4	7	28 GB	flyci-macos-large-latest-m2	$0.08	-
M2	8	14	28 GB	flyci-macos-xlarge-latest-m2	$0.16	-

500 mins/month Free for Public Repos

If your repo is public, then FlyCI offers 500 mins/month of free M1 runner usage with the flyci-macos-large-latest-m1 runner.

Best Regards, Kiril Gantchev CEO and co-founder of FlyCI

compiler-research / CppInterOp