-
Notifications
You must be signed in to change notification settings - Fork 278
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
Streamline MinGW package installation for CI #2004
Conversation
Codecov Report
@@ Coverage Diff @@
## 0.27-maintenance #2004 +/- ##
=================================================
Coverage 46.36% 46.36%
=================================================
Files 146 146
Lines 23005 23005
Branches 11809 11809
=================================================
Hits 10666 10666
Misses 6689 6689
Partials 5650 5650 Continue to review full report at Codecov.
|
Btw, I think we can remove pip and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with that change.
I wasn't aware of the env variable MSYSTEM. This sounds important and isn't in README.md
The code for i in base-devel .......
was cut'n'pasted from README.md. Maybe you should update README.md with your code and/or MSYSTEM. I didn't mention apt-cyg in README.md, because I only discovered it when I wrote appveyor_ming_cygwin.yml.
You mentioned something was "frozen". It makes sense to upgrade the tools on the CI at this time (between releases) and freeze them working on a release.
It's used by a few MSYS2/MinGW scripts to figure out what MSYS2 environment you're currently building for so the toolchain, paths, etc. are set properly. Perhaps those scrips are not used here, but might be best to play by the book. MSYSTEM is set by respective MINGW64/CLANG64 etc. launchers installed by the MSYS2 package (and by the setup-msys2 GHA) I'll update the README.md
It was just an assumption - I thought the info about package versions from the MSYS2/Cygwin repositories isn't updated (i.e. was set at the time of VM image creation by Appveyor), but then I saw in the logs that the there is some update happening and latest ones are being pulled from the repositories, so we can ignore this. |
As always, we're in enthusiastic agreement. I wasn't expecting to spend Thanksgiving working on this. Anyway, it's done. Have a great weekend. When you update README.md, can you update the date at the bottom of the file. |
f47a17f
to
ab9bc3e
Compare
Pull request has been modified.
I decided against setting MSYSTEM finally, as there's no login shell used in the build scripts here (which is where it matters as /etc/profile sets up a bunch of other stuff based on MSYSTEM value...); the PATH should indeed be everything that is needed in this particular case. |
ab9bc3e
to
d3f97a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, Miloš. I added a sentence to the Cygwin section in README.md about apt-cyg.
You mentioned something about python. If you want to remove something, that's fine. If the CI is green, the script is good enough. When I wrote appveyor_mingw_cygwin.yml about a year ago, I threw in the kitchen sink to get it working. I'm surprised it has been in service for a year without maintenance.
Kev @kevinbackhouse has a daily/main build. I believe he runs that occasionally on 0.27-maintenance. Those builds do not include mingw and/or cygwin. If we added mingw to those builds, we could discontinue publishing released builds on Exiv2.org. MinGW is popular with Windows/Qt developers (such as darktable). I added Cygwin about 10 years ago because I used Cygwin every day at work.
We can quietly drop Cygwin. If anybody needs Cygwin, they can open an issue about it.
I'm really pleased to be working with @nehaljwani on the release process and relocating exiv2.org to a new host. If we have daily builds of main and 0.27-maintenance, the web-site can be simplified to document their availability. A little team-work will result in a win-win. Better service for users, less work for Team Exiv2. Can you work with Kev to get MinGW into the daily build?
I forgot to say "Thank You" for the explanation about MSYSTEM. That's shell magic and necessary to get MinGW/msys2 to initialised correctly. I'm pleased to learn this because I have batch files (cmd64.bat, cygwin64.bat and msys2.bat) to initialise my environment when I power up bash.exe/cmd.exe. Those scripts don't mention MSYSTEM and set the PATH. These scripts are documented in README.md and README-CONAN.md Now that the test harness is 100% python, the environment to build/test exiv2 is less sensitive to the PATH. When parts of the test suite were written in bash, PATH magic was essential. |
I tried to create a GitHub Action that would build Exiv2 on Cygwin and MinGW, but I couldn't figure out how to make it work. My branch is here if you want to have go: https://github.com/kevinbackhouse/exiv2/tree/BuildCygwin |
I'll give the MinGW GHA on main a go one of these days, think they really tried to make it usable w/ setup-msys2. Not convinced we should keep supporting Cygwin CI and releases though - it is pure POSIX after all, and Linux testing should be sufficient there... |
I agree @kmilos with quietly dropping Cygwin from the build. It's very stable and doesn't deserve attention. As I said:
I don't want to create work by adding MinGW to the daily build. I'm hoping the extra effort on the daily build will save release engineering effort as we can discontinue publishing builds on exiv2.org. |
@clanmills You're welcome! You'll also find that the shell launcher Windows shortcuts shipped w/ the Msys2 installer do everything for you already in terms of all the necessary variable setup and are the preferred way of launching the MINGW64 environment and others. |
Oh, I agree there is no need to release MinGW (I already try to maintain the package in the msys2 repository), but a CI task to catch any regressions and problems related to that platforms is still a good idea. |
No description provided.