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 build and test ci #248

Closed
wants to merge 1 commit into from
Closed

Add build and test ci #248

wants to merge 1 commit into from

Conversation

avinal
Copy link

@avinal avinal commented Mar 2, 2022

Description

Added build and test CI for metacall/core using GitHub Actions for following platforms

  • Ubuntu 20.04, 18.04 GCC and Clang
  • Windows 2019 MSVC (failing, need help)
  • MacOS 11 Clang (failing, need help)

Fixes #(issue_no)

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests/screenshots (if any) that prove my fix is effective or that my feature works.
  • I have tested the tests implicated (if any) by my own code and they pass (make test or ctest -VV -R <test-name>).
  • If my change is significant or breaking, I have passed all tests with ./docker-compose.sh build &> output and attached the output.
  • I have tested my code with OPTION_BUILD_SANITIZER or ./docker-compose.sh test &> output and OPTION_TEST_MEMORYCHECK.
  • I have tested with Helgrind in case my code works with threading.
  • I have run make clang-format in order to format my code and my code follows the style guidelines.

If you are unclear about any of the above checks, have a look at our documentation here.

Tests/Logs

https://github.com/avinal/core-1/actions/runs/1921677717

Note for failing ci and suggestions

- added build-test ci
- added matrix for multiple platforms
- added name to contributors list

Signed-off-by: Avinal Kumar <avinal.xlvii@gmail.com>
@avinal
Copy link
Author

avinal commented Mar 2, 2022

Hello @viferga, It needs GitHub Actions to be enabled. I also need help with the Windows and macOS CI. Please let me know if you want more features to be added, i.e memory check, coverage etc.

@viferga
Copy link
Member

viferga commented Mar 16, 2022

Respect to this PR, I am not sure if we should merge it or not. The problem respect to this is that it is really hard to fully test MetaCall, and we are doing that only in Linux with a dockerized environment right now.

I have added the command ./docker-compose.sh test which does a testing based on sanitizers for detecting memory leak errors and similar.

I don't want to close this PR because it can serve as a base for future testing in other architectures, compilers or operative systems, but I cannot merge it yet because it feels incomplete to me.

Testing the metacall/distributable-{macos,linux,windows} can be also a good option for integration testing if we do not want to suffer installing all the dependencies in the CI for testing.

@avinal
Copy link
Author

avinal commented Mar 17, 2022

Hey @viferga, if this seems incomplete then don't merge it, also the overall result that you expect from CI/CD is very different from this PR, but yeah it would be nice to keep this around for future purposes. You can maybe add some appropriate labels to this PR to denote that.

For the integration testing, I think having docker containers with preinstalled dependencies and hosted on GitHub container registry is our best bet. It solves caching problems, reduces CI times by skipping installing dependencies, and you still get all the testing features.

I did something like this for one of my personal project and it reduced the CI duration from 1min 30seconds plus to under 30seconds, https://github.com/avinal/lark/pkgs/container/lark (image)

@avinal
Copy link
Author

avinal commented Mar 17, 2022

Meanwhile, I will keep an eye on CI/CD for metacall and will try something better suited to the needs if I get free time.

@viferga
Copy link
Member

viferga commented Jan 17, 2023

@avinal Hey mate we are working again on the CI, now Linux is working properly (it has been pretty difficult to implement) and we are working on Windows too. Are you interested in implementing MacOS?

@avinal
Copy link
Author

avinal commented Jan 19, 2023

Hi @viferga, I almost forgot that this PR was here 😅. I would love to implement it for macOS but can't promise since now I have a full-time job. Let me try it this weekend. 🤞🏼

@ahmedihabb2
Copy link
Contributor

ahmedihabb2 commented Feb 8, 2023

There are 3 tests that failed on MacOS which is:

  • 15- detour-test
  • 4 - log-custom-test (Subprocess aborted)
  • 35 - metacall-dynlink-path-test

You can view the logs from here https://github.com/ahmedihabb2/core/actions/runs/4126612989/jobs/7128770774

@viferga

@viferga
Copy link
Member

viferga commented Feb 8, 2023

@ahmedihabb2 can you create a new PR with your changes? There's things to review in your code.

@ahmedihabb2 ahmedihabb2 mentioned this pull request Feb 8, 2023
8 tasks
@ahmedihabb2
Copy link
Contributor

  • 15- detour-test
  • 4 - log-custom-test (Subprocess aborted)
  • 35 - metacall-dynlink-path-test

@ahmedihabb2 can you create a new PR with your changes? There's things to review in your code.

#383

@viferga
Copy link
Member

viferga commented Sep 13, 2023

@avinal I am closing this issue because as you can see we have advanced a lot with the CI. Also you will notice how complex this task is if you check the current implementation of all scripts and CI for solving it. There's still a lot to improve but at least we have covered a lot of cases and languages of MetaCall for MacOS and Windows. Linux support is complete, including sanitizers. It stills lacks valgrind but the project is not fully prepared to have it right now due to false positives from runtimes and lack of having support to it from the beginning with CI.

@viferga viferga closed this Sep 13, 2023
@avinal
Copy link
Author

avinal commented Sep 14, 2023

Hi @viferga, I understand, and it is a good effort to clear up outdated issues/PR. Thanks

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.

None yet

3 participants