CI: add Windows/MSYS2 package build/test

[umarcor]

While trying to update the Verilator package in MSYS2 (which is ~8 months behind), I found that compilation would fail.

In order to keep better track of this type of regressions, this PR adds MSYS2 testing to CI: https://github.com/verilator/verilator/actions/runs/411990309. A PKGBUILD recipe is added, which is similar to the upstream but builds the sources surrounding it, instead of downloading a tarball. Then, in one CI job a package is built and uploaded as an artifact. In a second job, the artifact is installed and tested, similarly to how any user would do in a clean environment.

GitHub Action msys2/setup-msys2 is used for setting up MSYS2 and for caching installed packages.

Currently, Linux jobs are failing because I need to add myself to the contributors list properly. However, this PR does not modify those at all.

---

The current build error on MSYS2 is the following:

../V3Os.cpp:353:31: error: 'WEXITSTATUS' was not declared in this scope; did you mean 'PNTSTATUS'?
  353 |         const int exit_code = WEXITSTATUS(ret);
      |                               ^~~~~~~~~~~
      |                               PNTSTATUS
make[2]: *** [../Makefile_obj:293: V3Os.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/d/a/verilator/verilator/msys2/src/build-x86_64-w64-mingw32/src/obj_opt'
make[1]: *** [Makefile:60: ../bin/verilator_bin] Error 2
make[1]: Leaving directory '/d/a/verilator/verilator/msys2/src/build-x86_64-w64-mingw32/src'
make: *** [Makefile:222: verilator_exe] Error 2

[wsnyder]

Can you please submit a pull to fix the WEXITSTATUS?

I don't want to add MINGW unless someone is willing to maintain it as this would distract from progress on Linux; are you willing to do so? FWIW Freebsd is a similar example where it lacks consistent love - and also the CI system kept randomly failing, so had to get disabled.

[wsnyder]

As to the patch... please make a new action rather then the normal build one, as I don't want builds to fail until this is proven stable.

Please remove the unicode in action names.

Does msys2/PKGBUILD have to be there, would like to avoid more top-level directories.

[umarcor]

Can you please submit a pull to fix the WEXITSTATUS?

I'm not a C guy... I could work around it with https://github.com/umarcor/verilator/commits/fix/WEXITSTATUS, but I don't think that's acceptable because of https://github.com/verilator/verilator/blob/master/src/V3Os.cpp#L53-L59. As far as I understand, even if WIFEXITED does not exist on MSYS2, it should be defined there...

I don't want to add MINGW unless someone is willing to maintain it as this would distract from progress on Linux; are you willing to do so? FWIW Freebsd is a similar example where it lacks consistent love - and also the CI system kept randomly failing, so had to get disabled.

I can help with my best effort. Tests will be run in hdl/MINGW-packages, so it's not critical to have CI added here. My main motivation for opening this PR was providing a reproducible example of the build issue, which you can easily see in a "known" environment (CI). So we can discuss the best approach for fixing it and have it checked straightaway.

As to the patch... please make a new action rather then the normal build one, as I don't want builds to fail until this is proven stable.

Done. I renamed the existing workflow to 'Build Linux' and I added 'Build Windows'. I updated the badges/shields in the README accordingly.

Please remove the unicode in action names.

Done.

Does msys2/PKGBUILD have to be there, would like to avoid more top-level directories.

It does not. I now moved it to ci/msys2/PKGBUILD.

[umarcor]

and also the CI system kept randomly failing, so had to get disabled

MSYS2, just as Arch Linux, is a rolling release. Therefore, failures should be expected every once in a while, since compilers and dependencies are bumped regularly. However, it is mostly stable. I'd say there are about ~4 temporary breaks a year.

[umarcor]

This PR now includes a patch to fix the WEXITSTATUS issue. I picked the define from https://github.com/bminor/binutils-gdb/blob/master/gdbsupport/gdb_wait.h#L79-L85 (see December 10, 2020 8:38 AM).

However, CI seems not to like my username. It works on the fork, tho:

• https://github.com/umarcor/verilator/actions/runs/412478028
• https://github.com/umarcor/verilator/actions/runs/412478027

[wsnyder]

Can you please separate the WIF part (& username), that seems better a separate commit than CI stuff and will merge it first, appreciated.

[wsnyder]

BTW should it be named "Build Linux"? We'll hopefully get OSX back up and perhaps FreeBSD, these could be under the same "Build Linux", though I suppose we could also break them up as separate actions too. Thoughts?

[umarcor]

Can you please separate the WIF part (& username), that seems better a separate commit than CI stuff and will merge it first, appreciated.

Done: #2685.

BTW should it be named "Build Linux"? We'll hopefully get OSX back up and perhaps FreeBSD, these could be under the same "Build Linux", though I suppose we could also break them up as separate actions too. Thoughts?

It can be named whatever you want. Honestly, I changed it for the sake of discussion. As per #2682 (comment) and #2683 (comment), there are only two motivations for having different workflows:

• Having separated triggering/event conditions (push, PRs, cron, dispatch...).
• Having separated shields/badges for showing the status.

Therefore, I think that having coverage.yml and build.yml does make sense. But Build Linux (build-lin.yml) and Build Windows (build-win.yml) is not necessary. Both of them are triggered by the same events, and any of them will turn the global CI status red. The only advantage is having two shields. However, it comes at the cost of having to browse two different "CI maps" (https://github.com/verilator/verilator/actions/runs/412478080 and https://github.com/verilator/verilator/actions/runs/412478081), instead of having all the "build and test" jobs in the same view.

Therefore, I would propose one of the following solutions:

• Merge build-lin.yml and build-win.yml into a single workflow again.
• Or, rename buil-lin.yml to tests.yml and build-win.yml to unstable-tests.yml. Then, the former will contain all the platforms/tests which are "critical" and the latter will contain all the platforms/tests which are expected to fail frequently, or which we are not confident about yet. In the future, be it macOS, FreeBSD or some test suite for "strange" features, we can put them in one or the other without "naming" issues.

[wsnyder]

I'd probably lean towards your idea of having a normal build (Linux), and extended build Mac/Windows/FreeBSD). Also if we can get it fast enough would be nice to have coverage reports on all pull requests, but that probably is better as a separate task.

[umarcor]

I rebased on top of master and I renamed the workflows to 'Tests' and 'Extended Tests'. I updated the shields accordingly: https://github.com/umarcor/verilator/blob/ci/msys2/README.adoc. I believe this PR is now ready.

Also if we can get it fast enough would be nice to have coverage reports on all pull requests, but that probably is better as a separate task.

That will be something to be done on linux only I think. That's because I/O on Windows is much slower. In GHDL's repo, for example, Windows is 2-5 times slower when running the test suites: https://github.com/ghdl/ghdl/actions/runs/407405565 (4min linux vs 19min windows, or 15-17min linux vs 42min windows).

[gezalore]

FWIW the name "Extended tests" is a bit unforunate as we already use that for the additional test suite under https://github.com/verilator/verilator_ext_tests

[umarcor]

I was about to update the MSYS2 packages to 4.204, but there seems to be some issue that prevents successful compilation on MINGW32: https://github.com/umarcor/verilator/runs/2858209508?check_suite_focus=true. However, building the current master is good: https://github.com/verilator/verilator/actions/runs/949678601. How should we proceed? Shall I cherry-pick some specific commits?

[umarcor]

FWIW the name "Extended tests" is a bit unforunate as we already use that for the additional test suite under https://github.com/verilator/verilator_ext_tests

I'm good with any alternative naming.

Anyway, note that maybe this PR won't be merged. That is, I'm good with having it permanently as an open PR, which I can bump every once in a while (when bumping in the MSYS2 repo fails). That allows reducing the CI usage in this org, and the traffic that maintainers of this repo need to handle.

[gezalore]

there seems to be some issue that prevents successful compilation on MINGW32

I believe that was fixed with 0c4d88b

[umarcor]

there seems to be some issue that prevents successful compilation on MINGW32

I believe that was fixed with 0c4d88b

It was!

• https://github.com/umarcor/MINGW-packages/actions/runs/950141596
• umarcor/MINGW-packages@bc9e6ed

Thank you so much 😉

[gezalore]

Glad to hear the patch worked out. BTW I was raising the naming because these jobs show up in the web UI, even if they are just on a PR: https://github.com/verilator/verilator/actions Took me a while to figure out which one was which 😄

[umarcor]

@gezalore, just let me know which name do you want to use instead. I will rename, and then I think the old one won't be shown.

[umarcor]

I tried updating the Verilator package in MSYS2 to 4.216, but I'm getting the following error:

../V3File.cpp: In member function 'void VInFilterImp::start(const string&)':
../V3File.cpp:482:19: error: assignment of read-only member 'VInFilterImp::m_pid'
  482 |             m_pid = 0;  // Disabled
      |             ~~~~~~^~~
make[2]: *** [../Makefile_obj:300: V3File.o] Error 1

https://github.com/verilator/verilator/runs/4425043216?check_suite_focus=true

According to

verilator/src/V3File.cpp

Lines 335 to 339 in ac05a77

	#ifdef INFILTER_PIPE
	pid_t m_pid = 0; // fork() process id
	#else
	const int m_pid = 0; // fork() process id - always zero as disabled
	#endif

and

verilator/src/V3File.cpp

Line 507 in ac05a77

const pid_t pid = fork();

, m_pid is constant indeed.

[umarcor]

I tried two solutions:

• umarcor@a0530e4
• 491d2b3

Both of them allow building and do pass the testsuite. @wsnyder which should I submit as a fix?

[wsnyder]

I pushed removing the const, thanks for reporting.

[wsnyder]

Replaced by #3814, #3819.