Switch MinGW CI to GHA #2019

kmilos · 2021-12-08T16:42:18Z

Also removed all mentions of EXIV2_EXT, not used since 0.27.4...

kmilos · 2021-12-08T16:46:45Z

Ah, looks like I was too eager on simplifying the appveyor script... Not too familiar w/ it, will fix up later.

An none of the Windows workflows are being run on PR, any ideas why?

clanmills

And I thought the Christmas had arrived early! Maybe you could mark the PR as draft until ready for review.

I'm not sure I can say anything useful about this as I am unfamiliar with GHA. It's good to see the code segmented and not a monolithic ugly little brute.

Question: When this is working, will it be triggered by the daily build with downloadable build artefacts?

kmilos · 2021-12-08T19:03:05Z

Question: When this is working, will it be triggered by the daily build with downloadable build artefacts?

This is just replacing existing "on PR" functionality and nothing else.

kmilos · 2021-12-08T19:09:17Z

And I thought the Christmas had arrived early!

Well, it's not working yet 😉

codecov · 2021-12-09T10:12:14Z

Codecov Report

Merging #2019 (11caec7) into 0.27-maintenance (2fb3f79) will not change coverage.
The diff coverage is n/a.

❗ Current head 11caec7 differs from pull request most recent head 95aa2ca. Consider uploading reports for the commit 95aa2ca to get more accurate results

@@                Coverage Diff                @@
##           0.27-maintenance    #2019   +/-   ##
=================================================
  Coverage             46.36%   46.36%           
=================================================
  Files                   146      146           
  Lines                 23005    23005           
  Branches              11809    11809           
=================================================
  Hits                  10666    10666           
  Misses                 6689     6689           
  Partials               5650     5650

Impacted Files	Coverage Δ
src/convert.cpp	`27.79% <ø> (ø)`
src/image.cpp	`49.25% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fb3f79...95aa2ca. Read the comment docs.

Fix appveyor

kmilos · 2021-12-09T11:00:33Z

Should be ready now... A few observations:

Had to turn off (i.e. leave off by default) EXIV2_TEAM_EXTRA_WARNINGS and EXIV2_TEAM_WARNINGS_AS_ERRORS - neither the Fedora MinGW on GHA nor the Appveyor MinGW jobs had them enabled
There is one failing test for MinGW but CI still says green? We can look into it, or skip unit tests - neither the Fedora MinGW on GHA nor the Appveyor MinGW jobs had them enabled (Appveyor just did python_tests subset for both MinGW and Cygwin)

clanmills

The unit test that's failing is benign. The message concerns a string coming from the C-runtime library:

    Which is: "Unknown error 9999 (errno = 9999)"
256
  strError().c_str()
257
    Which is: "Unknown error (errno = 9999)"

There's code somewhere to "massage and/or forgive" these differences. Its an issue in the test code, not exiv2.

I also see messages about "unsupported format". I thought the test harness ($ make unit_test) had code to pipe those ugly messages to /dev/null. Maybe that code isn't working on MinGW/msys2. Again, this is an issue with the test harness and not with the libexiv2.

I'm busy today and will investigate tomorrow. Let's see if Luis and/or Kev have other comments to make.

kmilos · 2021-12-09T13:39:54Z

Thanks Robin, let's see if there are ideas... As I said, we can alternatively just drop down to python_tests only like we have currently on AppVeyor.

Also, I left the same matrix as we have for MSVC (Release/Debug x shared/static), do we really want/need all of them? (We had [Release, Debug] x static) for the Fedora MinGW job, and Release + shared for the AppVeyor MinGW one...)

Finally, what do you think about giving Cygwin migration to GHA a go as well?

clanmills · 2021-12-09T14:12:12Z

I think we should leave the code make tests. The make pythontests might be < 50% of the tests.

I don't have time today to investigate the two anomalies in unit_tests about error 9999 and unsupported time format. Let's get them fixed in this PR.

Now that you've shown how to integrate the MinGW platform into the test harness, I'll do Cygwin/64 and follow your example. Can I add it to this PR (tomorrow) or submit a new PR (next week)?

kmilos · 2021-12-09T14:19:58Z

I think we should leave the code make tests. The make pythontests might be < 50% of the tests.

I don't have time today to investigate the two anomalies in unit_tests about error 9999 and unsupported time format. Let's get them fixed in this PR.

Agreed

Now that you've shown how to integrate the MinGW platform into the test harness, I'll do Cygwin/64 and follow your example. Can I add it to this PR (tomorrow) or submit a new PR (next week)?

I think I prefer we do it a step at a time to make it easier on ourselves.

clanmills · 2021-12-09T14:23:58Z

Right. I'll leave Cygwin/64 for next week (or later). Tomorrow I'll look at the 9999 and 'unsupported time format` and let you know. If Luis and Kev don't give feedback, we should get this closed tomorrow.

piponazo

great contribution! 👏

My only question is about why we are keeping the appveyor pipeline.

.github/workflows/on_PR_windows_matrix.yml

appveyor_mingw_cygwin.yml

.github/workflows/on_PR_windows_matrix.yml

piponazo · 2021-12-09T18:21:38Z

Also, I left the same matrix as we have for MSVC (Release/Debug x shared/static), do we really want/need all of them? (We had [Release, Debug] x static) for the Fedora MinGW job, and Release + shared for the AppVeyor MinGW one...)

IMHO I would keep it like you have done it. It is already implemented and I guarantee you that it will find subtle issues in the future. Furthermore it is not adding much additional processing time to the whole CI pipeline since the jobs are running in parallel. Better to be safe than sorry 😸

piponazo · 2021-12-10T08:21:19Z

The unit test that's failing is benign. The message concerns a string coming from the C-runtime library:

Regarding the failing test, I think we can simplify the test to be platform agnostic. Right now we are checking the exact string returned in each platform:

TEST(strError, doNotRecognizeUnknownError)
{
    errno = 9999;
    bool useExactString{true};
#if   defined(__MINGW__) || defined(__MSYS__) || defined(__CYGWIN__)
    const char * expectedString = "Unknown error 9999 (errno = 9999)";
#elif defined(_WIN32)
    const char * expectedString = "Unknown error (errno = 9999)";
#elif defined(__APPLE__)
    const char * expectedString = "Unknown error: 9999 (errno = 9999)";
#elif defined(__sun__)
    const char * expectedString = "Unknown error (errno = 9999)";
#elif defined(__FreeBSD__)
    const char * expectedString = "Unknown error: 9999 (errno = 9999)";
#elif defined(__NetBSD__)
    const char * expectedString = "Unknown error: 9999 (errno = 9999)";
#else
    // On Alpine we get 'No error information (errno = 9999)' instead of 'Unknown error 9999 (errno = 9999)'
    const char * expectedString = "(errno = 9999)";
    useExactString = false;
#endif
    if (useExactString) {
      ASSERT_STREQ(expectedString, strError().c_str());
    } else {
      ASSERT_TRUE(strError().find(expectedString) != std::string::npos);
    }
}

Probably it was me who started to write the test like that. At this point, I do not see the value of checking for the exact string.
Since the substring errno = 9999 seems to be always present in all the platforms I would just simplify the test to check that such substring is present in the error string:

TEST(strError, doNotRecognizeUnknownError)
{
    ASSERT_TRUE(strError().find("(errno = 9999)") != std::string::npos);
}

kmilos · 2021-12-10T08:32:25Z

Even if we fixed this test, what I'm also worried about is the fact that CI passed when there was a failing case.

piponazo · 2021-12-10T08:41:11Z

Even if we fixed this test, what I'm also worried about is the fact that CI passed when there was a failing case.

Right, that's more important.

I think the culprit of this situation is to use cmake --build to run the tests:

      - name: Test
        env:
          EXIV2_EXT: .exe
        run: |
          cd build
          cmake --build . --target tests

Probably cmake --build will succeed once the target is correctly built. On the main branch we are using CTest and I think CTest will correctly turn the CI job into red if a test fails:

      - name: Test
        if: ${{matrix.platform == 'x64'}}
        env:
          EXIV2_EXT: .exe
        run: |
          cd build
          ctest --output-on-failure

We should probably first fix this situation (either in this PR or in a separate one).

kmilos · 2021-12-10T09:38:27Z

So the commit 11caec7 was unsuccessful, and more substantial changes to CMake on 0.27-maintenance would be needed. Maybe out of scope for this PR?

piponazo · 2021-12-10T09:55:23Z

So the commit 11caec7 was unsuccessful, and more substantial changes to CMake on 0.27-maintenance would be needed. Maybe out of scope for this PR?

I have created the issue #2022 to take care about this. And yes, I think it is out of scope for this PR. I will try to take care of #2022 in the next days.

kmilos · 2021-12-10T10:20:15Z

Thanks. Best to leave any test fixing as part of #2022 solution as well then.

@clanmills Ok to merge as is then? You can then have a go at Cygwin on GHA on top of this...

clanmills · 2021-12-10T12:49:52Z

Great team-work. I'll merge. Luis will take care of #2022 and I'll look at Cygwin/64 next week.

Thanks Very Much, Everybody.

kmilos · 2021-12-10T12:51:53Z

Thanks everyone.

@kevinbackhouse Can probably cherry pick some stuff for main as well.

Remove legacy MinGW CI builds

d8466f7

kmilos requested review from clanmills and piponazo December 8, 2021 16:42

clanmills reviewed Dec 8, 2021

View reviewed changes

kmilos marked this pull request as draft December 8, 2021 23:04

kmilos force-pushed the ci_mingw_gha branch 5 times, most recently from 56cbaa5 to fd21b42 Compare December 9, 2021 09:49

Initial MSYS2 workflow for GHA

b1fea3d

Fix appveyor

kmilos force-pushed the ci_mingw_gha branch from fd21b42 to b1fea3d Compare December 9, 2021 10:35

kmilos marked this pull request as ready for review December 9, 2021 10:55

kmilos requested a review from kevinbackhouse December 9, 2021 11:39

clanmills reviewed Dec 9, 2021

View reviewed changes

piponazo reviewed Dec 9, 2021

View reviewed changes

.github/workflows/on_PR_windows_matrix.yml Show resolved Hide resolved

appveyor_mingw_cygwin.yml Show resolved Hide resolved

.github/workflows/on_PR_windows_matrix.yml Show resolved Hide resolved

Handle unused warnings

95aa2ca

kmilos force-pushed the ci_mingw_gha branch from e059bf8 to 95aa2ca Compare December 10, 2021 08:07

kmilos force-pushed the ci_mingw_gha branch from 11caec7 to 95aa2ca Compare December 10, 2021 09:19

piponazo mentioned this pull request Dec 10, 2021

CI for 0.27-maintenance not reporting test failures #2022

Closed

piponazo approved these changes Dec 10, 2021

View reviewed changes

kevinbackhouse approved these changes Dec 10, 2021

View reviewed changes

clanmills merged commit 2cbdfac into 0.27-maintenance Dec 10, 2021

clanmills deleted the ci_mingw_gha branch December 10, 2021 12:50

piponazo mentioned this pull request Dec 10, 2021

0.27 Running tests with ctest #2024

Merged

kmilos added the CI Issues related with CI jobs label Dec 11, 2021

kmilos mentioned this pull request Dec 17, 2021

Forward CI changes #2035

Merged

kmilos added this to the v0.27.6 milestone Dec 22, 2021

nehaljwani mentioned this pull request Jan 16, 2023

Is a new release happening soon? #2406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch MinGW CI to GHA #2019

Switch MinGW CI to GHA #2019

kmilos commented Dec 8, 2021 •

edited

Loading

kmilos commented Dec 8, 2021 •

edited

Loading

clanmills left a comment

kmilos commented Dec 8, 2021

kmilos commented Dec 8, 2021

codecov bot commented Dec 9, 2021 •

edited

Loading

kmilos commented Dec 9, 2021

clanmills left a comment

kmilos commented Dec 9, 2021

clanmills commented Dec 9, 2021

kmilos commented Dec 9, 2021

clanmills commented Dec 9, 2021

piponazo left a comment

piponazo commented Dec 9, 2021 •

edited

Loading

piponazo commented Dec 10, 2021

kmilos commented Dec 10, 2021

piponazo commented Dec 10, 2021

kmilos commented Dec 10, 2021

piponazo commented Dec 10, 2021

kmilos commented Dec 10, 2021

clanmills commented Dec 10, 2021

kmilos commented Dec 10, 2021

Switch MinGW CI to GHA #2019

Switch MinGW CI to GHA #2019

Conversation

kmilos commented Dec 8, 2021 • edited Loading

kmilos commented Dec 8, 2021 • edited Loading

clanmills left a comment

Choose a reason for hiding this comment

kmilos commented Dec 8, 2021

kmilos commented Dec 8, 2021

codecov bot commented Dec 9, 2021 • edited Loading

Codecov Report

kmilos commented Dec 9, 2021

clanmills left a comment

Choose a reason for hiding this comment

kmilos commented Dec 9, 2021

clanmills commented Dec 9, 2021

kmilos commented Dec 9, 2021

clanmills commented Dec 9, 2021

piponazo left a comment

Choose a reason for hiding this comment

piponazo commented Dec 9, 2021 • edited Loading

piponazo commented Dec 10, 2021

kmilos commented Dec 10, 2021

piponazo commented Dec 10, 2021

kmilos commented Dec 10, 2021

piponazo commented Dec 10, 2021

kmilos commented Dec 10, 2021

clanmills commented Dec 10, 2021

kmilos commented Dec 10, 2021

kmilos commented Dec 8, 2021 •

edited

Loading

kmilos commented Dec 8, 2021 •

edited

Loading

codecov bot commented Dec 9, 2021 •

edited

Loading

piponazo commented Dec 9, 2021 •

edited

Loading