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

This is a collection of commits that makes execution of arduino_ci customizable #320

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

Conversation

hlovdal
Copy link
Contributor

@hlovdal hlovdal commented Feb 4, 2022

There is already CUSTOM_INIT_SCRIPT which is hard coded to run via /bin/sh. I made possible to override with a CUSTOM_INIT_SCRIPT_SHELL variable if the user want to use powershell, perl, python or whatever instead.

I also added support for similar scripts to be run before and/or after each run of unit tests with ARDUINO_CI_PRE_UNIT_TEST_RUN_SCRIPT and ARDUINO_CI_POST_UNIT_TEST_RUN_SCRIPT (and similar ..._SHELL overrides). If set, the corresponding script will be run once for each configured platform and compiler combination, e.g. if .arduino-ci.yaml contains

unittest:
  compilers:
    - g++-10
    - g++-11
  libraries: ~
  platforms:
    - leonardo
    - uno

the scripts will be invoked four times, with the current platform name being tested as the first parameter to the script and the current compiler used as the second parameter.

Then there is ARDUINO_CI_UNIT_TEST_EXTRA_COMPILER_FLAGS to be able to inject extra compiler arguments when running tests (I use it to add --coverage -O0 -g -fprofile-abs-path).

@jgfoster
Copy link
Member

@ianfixes, This seems to be well-documented, isolated, backwards compatible, and useful (to at least the author!). Unless you have any objections in the next few days I'm inclined to merge it.

@ianfixes
Copy link
Collaborator

Hi @hlovdal, thanks very much for this contribution! I don't object to this being merged, but I do have some questions about this because it's possible that some existing features should be removed in favor of these (which can happen later).

CUSTOM_INIT_SCRIPT_SHELL is awesome, period. I'm glad to see that being added.

For the pre/post unit test scripts, are you adding these as a way to get hooks for unit test coverage? I think that is a feature worth adding at the top level, possibly by including some ruby support for --coverage. If this is your use case, let's talk about how to bring more of it into this project!

Similarly, are the compiler flags part of adding coverage to unit testing? There is a more targeted way to define compiler flags in YAML as part of the platform definition, but I get the sense that you are using them for something more "global" than this.

@hlovdal
Copy link
Contributor Author

hlovdal commented Mar 3, 2022

Sorry for a late and short (for now) answer. Yes my motivation for this is to use for unit tests, although I view the scripts to be very generic and usable for other things as well. For instance running a sonarqube scan and publishing the the result, or running avr-size and append some measurements to a csv file, or several other similar things that are not related to code coverage.

Now the compiler flag injection is probably mostly only relevant for code coverage, so I can leave that out for now if you want.

I think it is very difficult to add code coverage support in ruby that is usable for everyone. Both since it can be a bit of a pain to get working outside trivial hello world projects, and it is not obvious what tools to use since there are multiple frontends like lcov and gcovr (which I gave up getting to work and found fastcov instead which I got working...).

@ianfixes
Copy link
Collaborator

ianfixes commented Mar 5, 2022

Don't worry about the late answer, I really appreciate the way you are helping me think through all of this.

I think it is very difficult to add code coverage support in ruby that is usable for everyone.

That's fair, but I was actually thinking the reverse: each unit test file creates its own executable, so for a proper assessment of coverage you'd need a separate process (after arduino_ci) to combine all the results together. Are you running into that? I'm just curious how you're managing it.

Overall I'm sold on the idea of what's being done here, I'm just wondering whether we should go further and supply args or env vars to the hooks you're writing.

@jgfoster
Copy link
Member

jgfoster commented Sep 2, 2022

@ianfixes,

Overall I'm sold on the idea of what's being done here, I'm just wondering whether we should go further

Rather than wait for "further" work, it seems like this could be merged and supplemented later. Any objections?

@ianfixes
Copy link
Collaborator

Picking up this thread again, TL;DR I like this idea but please do not merge this PR in its present state.

Overall I think the desired features align with the architecture of this project, but I want to really dive into the way in which we offer this customization to make sure that it can (a) run as a github action -- which limits how complex certain settings can be, and (b) be generic/flexible enough to support something like a 3rd party coverage tool.

My next step for this PR is to look at enhancing the way the script shell is executed, to see if there are any other customization features that make sense.

@hlovdal hlovdal force-pushed the scripts branch 2 times, most recently from addca09 to ff3c03b Compare December 28, 2022 16:13
@ianfixes
Copy link
Collaborator

@hlovdal is there a public repo where you're using the script setup that's enabled by this patch? I'm still trying to wrap my head around how it's being used, something seems "off" to me.

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