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

CI setup #395

Merged
merged 2 commits into from
Sep 6, 2022
Merged

CI setup #395

merged 2 commits into from
Sep 6, 2022

Conversation

rokups
Copy link
Contributor

@rokups rokups commented Sep 1, 2022

I would like to propose a more complete CI setup than #393. I used a minimal CMake script to simplify building across multiple platforms. This script is treated purely as CI facility script and is put to .github subdir in order to discourage users from using it.

Note that i included my PR #392 in this PR. I enabled max warning level on all platforms, so in order for build to succeed i needed those warnings fixed.

Tested platforms: Windows / Linux / MacOS
Tested compilers: msvc / mingw / gcc / clang
Tested architectures: x86 / x64
Sample build: https://github.com/rokups/implot/actions/runs/2972494310

@epezent
Copy link
Owner

epezent commented Sep 6, 2022

Thanks Rokas! This looks great. This is definetely something I've wanted to do but have not had time. So I'm more than happy to accept your solution. Since I am more familiar with CMake, I feel better merging this PR than #393 in case I need to make updates later down the road (@ozlb, thank you for your submission too!)

I don't have any immediate questions or concerns, so I will go ahead and merge this work.

@epezent epezent merged commit fd16fe0 into epezent:master Sep 6, 2022
@epezent
Copy link
Owner

epezent commented Sep 6, 2022

Actually, I have one question -- is it possible to setup the CI so that jobs are triggered when changes are made to ocornut/imgui (so that we are covered by our changes and ImGui's changes)?

@rokups
Copy link
Contributor Author

rokups commented Sep 7, 2022

Once test engine is public i will also add some muscle-flexing to the CI, basic tests that expand every demo example 🚀

Thing with testing upstream commits is a little complicated. We have two options:

  1. Have ocornut/imgui repo send a workflow dispatch event to epezent/implot and do builds on every push to upstream.
  2. Do CI builds on schedule, say every day or every week for example.

1st option sounds perfect, but recently i could not get it working, even though i did get it working in the past. Either i missed something obscure, or github changed their API and broke it. I will be looking into it once again later and if i succeed i will contribute it to implot.

2nd option is very simple to implement. All you have to do is to add on: schedule section in yml:

on:
  <other stuff here>
  schedule:
    - cron: '40 8 * * *'

It follows standard cron syntax. Downside is of course that builds will run even when there are no commits pushed to upstream master branch for multiple days.

@ocornut
Copy link
Collaborator

ocornut commented Sep 7, 2022

Thanks for merging !

I personally wasn't too sure about the cmake bits and suggested to rokups to try a version without, which you can see here:
c76c88f
I personally think this is slightly better but Rokas disagree. I guess they both are nice, just wanted to point out this commit existed somewhere.

One thing I would suggest adding is an extra build with IMGUI_DISABLE_OBSOLETE_FUNCTIONS and IMGUI_DISABLE_OBSOLETE_KEYIO defined. I don't think it needs to be done for all variants and compilers, this is merely a way for ImPlot to catch on those changes.

ImPlot being in the particular case of being a non-trivial yet commonly used extension, I guess it should keep putting some effort at using IMGUI_VERSION_NUM checks here and there, but you might eventually also consider to drop some code-path after a given time (e.g. I'm using 2 years).

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