epezent / implot

Immediate Mode Plotting
MIT License
4.65k stars 517 forks source link

CI setup #395

Closed rokups closed 2 years ago

rokups commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

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 commented 2 years ago

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: https://github.com/epezent/implot/commit/c76c88f6727d6457c9c05c70adf67adea85923a2 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).