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

Switch Cygwin CI to GHA #2032

Merged
merged 1 commit into from
Dec 17, 2021
Merged

Switch Cygwin CI to GHA #2032

merged 1 commit into from
Dec 17, 2021

Conversation

kmilos
Copy link
Collaborator

@kmilos kmilos commented Dec 17, 2021

No description provided.

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 17, 2021

Duh, the default shell for Cygwin action is PowerShell, trying its multi-line break instead...

Edit: Switched to bash eventually.

Also, not sure why AppVeyor is still attempting to run when the recipe has been removed?

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks about right. Let's see how/if it works out on the CI.

You're using the correct approach to install the dependencies and build with CMake without using conan. I've never succeeded in building with conan on Cygwin.

The code in appveyor_mingw_cygwin.yml use apt-cyg to install the dependencies and builds with cmake. Appveyor runs the cmd shell.

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 17, 2021

Yeah, and had to turn unit test off (same on AppVeyor) as there is not gtest on Cygwin available ootb.

The action is made by the same person who did the action for MSYS2 setup (and incidentally the VS shell), so should be usable 🤞

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 17, 2021

Almost there, now just a couple of tests fail, presumably because of line ending difference: ctest is run from PowerShell and not from bash like it was on AppVeyor...

@clanmills
Copy link
Collaborator

Here are the notes I made when I created this last year: https://help.appveyor.com/discussions/questions/51961-building-on-mingw-64-and-cygwin64

When this PR flies, we don't need appveyor to run appveyor_mingw_cygwin.yml at all. I can't remember how this is triggered. I'm fairly sure I had to register something on the appveyor web-site.

@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Merging #2032 (de71680) into 0.27-maintenance (959f452) will not change coverage.
The diff coverage is n/a.

❗ Current head de71680 differs from pull request most recent head 589079a. Consider uploading reports for the commit 589079a to get more accurate results
Impacted file tree graph

@@                Coverage Diff                @@
##           0.27-maintenance    #2032   +/-   ##
=================================================
  Coverage             58.74%   58.74%           
=================================================
  Files                   146      146           
  Lines                 23004    23004           
  Branches              12596    12596           
=================================================
  Hits                  13513    13513           
  Misses                 6689     6689           
  Partials               2802     2802           

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 959f452...589079a. Read the comment docs.

@kmilos kmilos force-pushed the ci_cygwin_gha branch 2 times, most recently from 914aaf8 to 4edcccb Compare December 17, 2021 12:09
@clanmills
Copy link
Collaborator

@kmilos Appveyor is running the cmd.exe (DOS console) and I execute bash to run the bash build script. I build the bash build script line-by-line with ; command separators. The bash build script is a one-liner.

set CMD=mkdir build
set CMD=CMD=%CMD%; cd build
...
C:\cygwin64\bin\bash -c "%CMD%"

@clanmills
Copy link
Collaborator

I'm fairly sure the build is failing in Appveyor because you've deleted appveyor_mingw_cygwin.yml. I think I had to register something with appveyor to tell him to use that file.

I remember thinking "this will only work on 0.27-maintenance". Ummm. I can't remember how it works and I can't find anything on appveyor. @kevinbackhouse Do you know how appveyor knows to read the file appveyor_mingw_cygwin.yml?

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 17, 2021

Thanks Robin, I was well aware of that. The problem was that the default bash launcher suggested by the action author changes the working directory to $HOME even w/ --norc, so I had to remove the --login option.

But we're back to square one (same as PowerShell), there are still a couple of failing tests...

@clanmills
Copy link
Collaborator

@kmilos.

I haven't understood what you are saying about the shell. Most of the tests pass, so it's mostly working fine. This looks like a solid/repeatable error. I'll try this after lunch on Windows.

At the moment, I'm giving README.md a thorough overhaul to deal with ctest.

@clanmills
Copy link
Collaborator

I've found the appveyor settings. https://ci.appveyor.com/project/clanmills/exiv2/settings. I will delete the project.

Later today, I'll investigate the other build failures on Windows. There can't be much wrong.

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 17, 2021

I've found the appveyor settings. https://ci.appveyor.com/project/clanmills/exiv2/settings. I will delete the project.

Please wait until we get this working ;)

The suggested workaround to check out Unix line endings also doesn't work currently... I'm taking a break from this one...

@clanmills
Copy link
Collaborator

Whoops. I've already deleted it.

I asked in a review for that code concerning the line-endings to be changed and the engineer refused. He's the only contributor I have ever fired.

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 17, 2021

Ok, figured out why the git config wasn't working (executed in the wrong shell), so this is my last attempt before touching tests.

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 17, 2021

Bingo!

Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Finally all the pipelines running on GHA! 🎊

@kmilos kmilos merged commit 6833eca into 0.27-maintenance Dec 17, 2021
@kmilos kmilos deleted the ci_cygwin_gha branch December 17, 2021 15:28
@clanmills
Copy link
Collaborator

You're almost as smart as I used to be!

Do you know the Country Song "As Good As I One Was"? https://www.youtube.com/watch?v=ldQrapQ4d0Y

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 17, 2021

Can't say I did :D

@clanmills
Copy link
Collaborator

"I'm not as good as I once was.
But I'm as good once, as I ever was!"

@kmilos kmilos mentioned this pull request Dec 17, 2021
@kmilos kmilos added the CI Issues related with CI jobs label Dec 20, 2021
@kmilos kmilos added this to the v0.27.6 milestone Dec 22, 2021
antermin pushed a commit to antermin/exiv2 that referenced this pull request Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Issues related with CI jobs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants