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 GitHub Action CI builds #1685

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

Conversation

JulianGro
Copy link
Contributor

@JulianGro JulianGro commented Jul 11, 2021

This PR adds GitHub Action CI builds which automatically test incoming Pull Requests and commits to master on multiple Operating Systems and configurations.

Because of #1684 and because I haven't figured out how to install libhunspell-dev on Windows and MacOS, both of those don't work yet.
QtWebKit with Qt 5.15.2 also doesn't work because I haven't figured out how to install QtWebKit without installing Qt from APT as well.

For new contributors, you need to allow running GitHub Actions by opening the "Checks" tab and pressing the button on the top right.

I am up for maintaining this, and maybe even adding proper packaging and uploading in the future.

@Frenzie
Copy link
Member

Frenzie commented Jul 11, 2021

Because of #1684 and because I haven't figured out how to install libhunspell-dev on Windows and MacOS, both of those don't work yet.

The -dev is a Debian-specific suffix for indicating the source package. It's otherwise meaningless.

You just grab something like https://github.com/hunspell/hunspell/files/2573619/hunspell-1.7.0.tar.gz and put the contents in an include path where it can find it.

@JulianGro
Copy link
Contributor Author

You just grab something like https://github.com/hunspell/hunspell/files/2573619/hunspell-1.7.0.tar.gz and put the contents in an include path where it can find it.

Well that hasn't been nearly as easy as it sounded.
I gave up for now on both MacOS and Windows. Setting Hunspell_INCLUDE_DIR and Hunspell_LIBRARIES hasn't worked, finding the include directory for Windows seems impossible, and cmake failed to find Hunspell in multiple standard include directories on MacOS.
See JulianGro#3 if you want to take a look at where I gave up.

@Frenzie
Copy link
Member

Frenzie commented Jul 12, 2021

CMake being CMake, that'd have to be something like cmake -DHunspell_INCLUDE_DIR. Also you'd have to compile it first.

Well that hasn't been nearly as easy as it sounded.

Apologies, I assumed you were at ease with that kind of thing given this PR. :-)

@JulianGro
Copy link
Contributor Author

I have at least gotten MacOS to work now, and not given up on Windows yet.

Also since the GitHub Actions don't have access to any sensitive information, you can just enable them for everyone (that doesn't have a fairly new GitHub account) under the "Actions" tab in the repository settings.

Try all major OS' with and without Hunspell
Cache Qt
Try building with Hunspell on Windows (fails)
@JulianGro
Copy link
Contributor Author

JulianGro commented Jul 13, 2021

I haven't been able to manually include hunspell. It is never able to find it for some reason. The failing Windows include hunspell code is in the last commit if someone wants to take a look. Otherwise the PR is as far as I want it to be.

@Emdek
Copy link
Member

Emdek commented Aug 17, 2021

@JulianGro, sorry for late reply.
I'm wondering how familiar are you with this mechanism, what are the options to extend it in future (AFAIK it would be even possible to use the artifacts for releases).

@Frenzie
Copy link
Member

Frenzie commented Aug 17, 2021

(AFAIK it would be even possible to use the artifacts for releases).

Yup! :-)

@JulianGro
Copy link
Contributor Author

Anything keeping this from getting merged?

- name: Build Otter Browser
working-directory: ${{runner.workspace}}/build
shell: bash
run: cmake --build . --parallel 3
Copy link
Member

Choose a reason for hiding this comment

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

I assume you based this on something, but I also assume GHA has a proper variable for it instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume you mean the thread count? Yes: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources
I don't think there is a GHA specific variable for the thread count, but my reasoning for that assumption is that you could just get the thread count from the system instead. We have 3 threads hardcoded on the Vircadia GHA which I adopted here.

Copy link
Member

Choose a reason for hiding this comment

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

It just doesn't seem very forward thinking. I'd rather see for example $(nproc) or something along those lines. (Presumably this is nproc + 1; is that better somehow?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that you talk about $(nproc) I understand why you were talking about a GHA variable and why we have it hardcoded on the Vircadia actions. I forgot that $(nproc) will only work on Linux (and maybe macOS).
GHA doesn't have a variable for the available threads.

@Frenzie
Copy link
Member

Frenzie commented Dec 25, 2021

All looks pretty good to me in any case. ;)

@Emdek
Copy link
Member

Emdek commented Dec 27, 2021

@JulianGro, for me the main issue is that I'm not really familiar with GHA yet…

@JulianGro
Copy link
Contributor Author

Well, you don't need to be.

@Emdek
Copy link
Member

Emdek commented Dec 30, 2021

@JulianGro, but it helps, a lot, especially when it can be used to produce release binaries.

@Emdek
Copy link
Member

Emdek commented Apr 5, 2022

@JulianGro, I've found someone that might help with this review, so hopefully I'll be finally able to deal with this PR. :-)

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