CI: add Github CI by pjonsson · Pull Request #247 · intel/tinycbor

pjonsson · 2023-11-02T18:45:20Z

No description provided.

pjonsson · 2023-11-02T18:56:55Z

My plan was to make a PR of the first commit, see what fails, and then add some commits that disables -Werror/some configurations/etc so the CI starts going green. I'm not sure why the jobs don't run automatically.

If this is merged, the jobs will be red for reasons I believe are unrelated to the CI:

There is no bool type in C90 and some tests are compiled as that
Incompatible -Werror=something flag on macOS
The linux-gcc-non-math test requires math.h in cborinternal_p.h (ldexp. INFINITY, NAN)
Implicit function declaration of cbor_value_to_pretty_advance_flags in src/cbortojson.c and tools/cbordump/cbordump.c
Probably some other warning that I have forgotten

thiagomacieira

Thank you! This is very much appreciated.

My plan was to make a PR of the first commit, see what fails, and then add some commits that disables -Werror/some configurations/etc so the CI starts going green. I'm not sure why the jobs don't run automatically.

They probably need to be merged first. But meanwhile we can see the results in your fork: https://github.com/pjonsson/tinycbor/actions

There is no bool type in C90 and some tests are compiled as that

That's not showing up in your fork. Do you have any details?

Incompatible -Werror=something flag on macOS

Ditto.

The linux-gcc-non-math test requires math.h in cborinternal_p.h (ldexp. INFINITY, NAN)

That one I can see. That requires a fix in TinyCBOR itself, so we can merge the CI and then fix on top.

Implicit function declaration of cbor_value_to_pretty_advance_flags in src/cbortojson.c and tools/cbordump/cbordump.c

Not seeing this either, but probably easy to fix.

Probably some other warning that I have forgotten

thiagomacieira · 2023-11-03T23:09:20Z

.github/dependabot.yml

+version: 2
+updates:
+- package-ecosystem: github-actions
+  directory: "/"
+  schedule:
+    interval: "daily"
+  target-branch: "main"


Does the dependabot even work for C code? TinyCBOR has no bundled dependencies (which is the correct way to develop software) so Dependabot not going to find anything even if it works for C code.

I'm not sure of the complete capabilities of Dependabot, it supports npm, gradle, maven, and a bunch of other ecosystems.

The part configured in this PR is the ecosystem "github-actions", to keep the Github action versions in this CI up to date.

Ok. I don't see the point, but it won't hurt.

.github/workflows/build.yml

thiagomacieira · 2023-11-03T23:11:55Z

.github/workflows/build.yml

+    # Install in /usr/bin so the ccache action gets the expected environment.
+    - name: install ccache
+      if: matrix.os == 'ubuntu-latest'
+      run: |
+        wget -nv https://github.com/ccache/ccache/releases/download/v4.8.3/ccache-4.8.3-linux-x86_64.tar.xz
+        sudo tar xf ccache-4.8.3-linux-x86_64.tar.xz -C /usr/bin --strip-components=1 --no-same-owner ccache-4.8.3-linux-x86_64/ccache
+        rm -f ccache-*-linux-x86_64.tar.xz
+
+    - name: ccache
+      uses: hendrikmuhs/ccache-action@v1.2.10
+      with:
+        key: compilation-${{ matrix.os }}-${{ matrix.build_cfg.name }}
+        max-size: 160M


TinyCBOR is tiny. How much do we gain by having this cached?

Last time I looked, the Github CI machines were dual core Xeon with a low clock frequency and slow IO. So the gain is noticeable.

Let me see if I am allowed to accept this third-party action. I think "github.com/intel" may have disabled all third-party actions.

Sorry, I can't accept this action. It needs to be "actions/" only or a few options.

pjonsson · 2023-11-03T23:41:07Z

My plan was to make a PR of the first commit, see what fails, and then add some commits that disables -Werror/some configurations/etc so the CI starts going green. I'm not sure why the jobs don't run automatically.

They probably need to be merged first. But meanwhile we can see the results in your fork: https://github.com/pjonsson/tinycbor/actions

There is no bool type in C90 and some tests are compiled as that

That's not showing up in your fork. Do you have any details?

Depending on which job you are looking at, it's possible you're looking at a branch where -Werror is more or less disabled, bool is typedefed to int when compiling pre-C99 standards, and qmake on macOS is from qt6.

I don't have a language standard handy, but https://en.cppreference.com/w/c/keyword/_Bool gives some indication that it was C99 that introduced the boolean type.

The build error is on macOS, but the job linked below times out because it takes >45 minutes to build qt5 on macos-11 (macos-11 is no longer supported by homebrew, and qt5 gives angry warnings about not being tested on the default SDK in the macos-latest image (which I think is macos-12)). I'm not sure why qmake from qt6 is prohibited in tinyCBOR, but I think it's easier to fix whatever is required to use qmake from qt6, so one gets binary qt packages from homebrew.

Incompatible -Werror=something flag on macOS

Ditto.

https://github.com/pjonsson/tinycbor/actions/runs/6739438498/job/18320911265

Edit: apparently I confused myself with the -Werror=something issues, it's Clang on Linux, not macOS.

thiagomacieira · 2023-11-06T16:49:12Z

Depending on which job you are looking at, it's possible you're looking at a branch where -Werror is more or less disabled, bool is typedefed to int when compiling pre-C99 standards, and qmake on macOS is from qt6.

I decided that the API would use bool, so we basically require C99 as a minimum. I don't care about C89 right now. If anyone wants to submit patches that remove warnings for those, feel free.

I don't have a language standard handy, but https://en.cppreference.com/w/c/keyword/_Bool gives some indication that it was C99 that introduced the boolean type.

It was.

The build error is on macOS, but the job linked below times out because it takes >45 minutes to build qt5 on macos-11 (macos-11 is no longer supported by homebrew, and qt5 gives angry warnings about not being tested on the default SDK in the macos-latest image (which I think is macos-12)). I'm not sure why qmake from qt6 is prohibited in tinyCBOR, but I think it's easier to fix whatever is required to use qmake from qt6, so one gets binary qt packages from homebrew.

I can look into that.

pjonsson · 2023-11-06T17:56:41Z

Depending on which job you are looking at, it's possible you're looking at a branch where -Werror is more or less disabled, bool is typedefed to int when compiling pre-C99 standards, and qmake on macOS is from qt6.

I decided that the API would use bool, so we basically require C99 as a minimum. I don't care about C89 right now. If anyone wants to submit patches that remove warnings for those, feel free.

Well, I personally don't care about pre-C99, but I have a patch for bool on pre-C99 since the C90-test doesn't pass otherwise.

The build error is on macOS, but the job linked below times out because it takes >45 minutes to build qt5 on macos-11 (macos-11 is no longer supported by homebrew, and qt5 gives angry warnings about not being tested on the default SDK in the macos-latest image (which I think is macos-12)). I'm not sure why qmake from qt6 is prohibited in tinyCBOR, but I think it's easier to fix whatever is required to use qmake from qt6, so one gets binary qt packages from homebrew.

I can look into that.

I have a patch for switching macOS to qmake from QT 6 as well. I don't have a mac though, so I can only see that the tests pass that were present in Travis still pass, everything else might crash and burn when switching to QT 6 for all I know.

I don't consider the mentioned patches as fixing CI issues though.

thiagomacieira · 2023-11-26T00:51:14Z

Well, I personally don't care about pre-C99, but I have a patch for bool on pre-C99 since the C90-test doesn't pass otherwise.

Please submit it. We can discuss whether to accept in the review.

The build error is on macOS, but the job linked below times out because it takes >45 minutes to build qt5 on macos-11 (macos-11 is no longer supported by homebrew, and qt5 gives angry warnings about not being tested on the default SDK in the macos-latest image (which I think is macos-12)). I'm not sure why qmake from qt6 is prohibited in tinyCBOR, but I think it's easier to fix whatever is required to use qmake from qt6, so one gets binary qt packages from homebrew.

I can look into that.

I have a patch for switching macOS to qmake from QT 6 as well. I don't have a mac though, so I can only see that the tests pass that were present in Travis still pass, everything else might crash and burn when switching to QT 6 for all I know.

I've fixed the Clang issue. Please rebase to see it.

pjonsson · 2023-11-26T09:31:44Z

The bool fix is in #249.

I've fixed the Clang issue. Please rebase to see it.

Rebase against what branch?

@pjonsson

It doesn't know -Wdiscarded-qualifiers. Reported by @pjonsson on #247. I really need to switch to CMake - #242 Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>

thiagomacieira · 2023-11-27T00:28:14Z

Rebase against what branch?

main. The change is ... uh... where did it go? Oops, it failed and I didn't notice.

Done now, as 0d55382 in main

Dependabot can provide automatic pull requests for things in the repository that should be updated. Example PR from dependabot against a repo owned by the github organization: github/opensource.guide#2248 Documentation: https://docs.github.com/en/code-security/dependabot/dependabot-security-updates/about-dependabot-security-updates

mofosyne · 2024-04-29T05:18:11Z

Oh is this PR already done, as 0d55382 in main ? If so then should this PR be closed then?

(Had a look previously as a request, but wasn't able to find a solution.)

pjonsson · 2024-04-29T05:57:56Z

This PR adds Github CI, 0d55382 makes the Makefile work with Clang.

thiagomacieira · 2024-05-01T23:43:04Z

Right. According to the CI logs in your repository, one of the builds is failing.

I'll merge as-is and then we can figure out how to fix it. It's better than no CI.

pjonsson · 2024-05-02T06:58:14Z

The Github macOS CI runners for "macos-latest" have changed CPUs from x86-64 to M1/M2 in the last month(s), so it's possible things related to macOS broke (more) after I made this PR.

thiagomacieira · 2024-05-02T14:17:38Z

I switched to macos-13 so Valgrind would run... but Valgrind is broken anyway, so I removed that.

pjonsson force-pushed the add-github-ci branch 4 times, most recently from 6fe2713 to 5fc850b Compare November 2, 2023 23:53

thiagomacieira reviewed Nov 3, 2023

View reviewed changes

thiagomacieira mentioned this pull request Nov 4, 2023

Fixing CI/CD #245

Open

thiagomacieira added a commit that referenced this pull request Nov 27, 2023

Makefile: Fix build with Clang

0d55382

It doesn't know -Wdiscarded-qualifiers. Reported by @pjonsson on #247. I really need to switch to CMake - #242 Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>

pjonsson force-pushed the add-github-ci branch from 5fc850b to 5a0e8b4 Compare November 27, 2023 08:24

CI: add Github CI

ae6bef3

pjonsson force-pushed the add-github-ci branch from 5a0e8b4 to ae6bef3 Compare November 27, 2023 08:26

thiagomacieira merged commit 4dcad26 into intel:main May 1, 2024

pjonsson deleted the add-github-ci branch May 2, 2024 06:54

Comments

Conversation

pjonsson commented Nov 2, 2023

Uh oh!

pjonsson commented Nov 2, 2023

Uh oh!

thiagomacieira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjonsson commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thiagomacieira commented Nov 6, 2023

Uh oh!

pjonsson commented Nov 6, 2023

Uh oh!

thiagomacieira commented Nov 26, 2023

Uh oh!

pjonsson commented Nov 26, 2023

Uh oh!

thiagomacieira commented Nov 27, 2023

Uh oh!

mofosyne commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pjonsson commented Apr 29, 2024

Uh oh!

thiagomacieira commented May 1, 2024

Uh oh!

pjonsson commented May 2, 2024

Uh oh!

thiagomacieira commented May 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pjonsson commented Nov 3, 2023 •

edited

Loading

mofosyne commented Apr 29, 2024 •

edited

Loading