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

Add MacOS to CI pipeline #120

Merged
merged 7 commits into from
May 25, 2020
Merged

Add MacOS to CI pipeline #120

merged 7 commits into from
May 25, 2020

Conversation

adamreeve
Copy link
Contributor

@adamreeve adamreeve commented May 21, 2020

Also addresses other related comments from @jgiannuzzi on #117.

I wonder what people think about using -latest vs specific OS versions? I had copied what Parquet sharp used with ubuntu-18.04 and windows-latest but seems a bit inconsistent to fix the Ubuntu version and not Windows and Mac.

Am making this as a draft PR for now as the Mac build seems to be hanging at compiling the T4 templates so need to investigate what's happening there (https://github.com/adamreeve/ILGPU/runs/695667380?check_suite_focus=true).

Edit: Also looks like the MacOS hosts will need the 2.1 runtime installed.

@adamreeve adamreeve marked this pull request as draft May 21, 2020 08:18
@jgiannuzzi
Copy link
Contributor

jgiannuzzi commented May 21, 2020

Thanks for this PR, @adamreeve!

About using -latest vs specific OS versions: as this project does not depend on any native code, I would suggest we use -latest everywhere.

About T4: that's weird, as it works on my mac with the same OS and .NET Core versions. Moreover I see it worked in the run for this PR: https://github.com/m4rs-mt/ILGPU/pull/120/checks?check_run_id=695686181.

One more thing: it looks like we should actually setup .NET Core 2.1 on macOS (and ideally on Linux to be consistent). Have a look at https://github.com/G-Research/ParquetSharp/blob/9dc36fad3e4b3b916a84f60248df57eaeff1bb80/.github/workflows/ci.yml#L63 for an example.

@adamreeve
Copy link
Contributor Author

Hmm yeah not sure what happened with the hanging step then, hopefully a random one-off thing...

Regarding the SDK setup, I actually tried that originally with Linux but it failed as I'm using "dotnet tool restore" which is only available since the 3.0 SDK, and that setup-dotnet action doesn't have support for installing different SDK and runtime versions side by side. Ideally we could use the 3.1 SDK with 2.1 runtime. It just works on Windows and Linux as there GitHub has the hosts set up with all the required SDK and runtime versions. I think I should be able to work around that by just installing the T4 tool explicitly each time rather than using "dotnet tool restore"

@jgiannuzzi
Copy link
Contributor

Not sure about this, but what about running that action twice, once with 2.1, and then once with 3.1?

@m4rs-mt
Copy link
Owner

m4rs-mt commented May 21, 2020

@adamreeve Hmm strange; We can also update the .Net Core version of all test cases if this helps to avoid the common problems. This should not hurt us, as ILGPU supports .Net Standard 2.0 and higher. We could even think about upgrading the .Net Core version of ILGPU, as well.

@adamreeve
Copy link
Contributor Author

Installing both SDK versions can work but needs a workaround to copy them into the same location, there's an issue open for this at actions/setup-dotnet#25. Or I could install the 3.1 SDK to set up and run the T4 tool, then switch to the 2.1 SDK to run the tests, but that feels a bit hacky too.

Upgrading the test projects to 3.1 would be the nicest option from my point of view if that won't be too much pain for other developers who might need to then install the new SDK. Otherwise I'm happy to just change the workflow to install T4 as a global tool in the CI build which will work with the 2.1 SDK.

@adamreeve adamreeve marked this pull request as ready for review May 25, 2020 01:07
@m4rs-mt
Copy link
Owner

m4rs-mt commented May 25, 2020

Installing both SDK versions can work but needs a workaround to copy them into the same location, there's an issue open for this at actions/setup-dotnet#25. Or I could install the 3.1 SDK to set up and run the T4 tool, then switch to the 2.1 SDK to run the tests, but that feels a bit hacky too.

This sounds like workarounds that are not really necessary. Let's take the upgrade approach, which shouldn't hurt many developers too much and makes the test integration process much easier.

Copy link
Owner

@m4rs-mt m4rs-mt left a comment

Choose a reason for hiding this comment

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

@adamreeve Awesome work 🥇

@m4rs-mt m4rs-mt merged commit 5f3c7cd into m4rs-mt:master May 25, 2020
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.

3 participants