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

CI: test on MINGW64 and MINGW32, use PKGBUILD recipe #398

Merged
merged 15 commits into from
Dec 3, 2020

Conversation

umarcor
Copy link
Contributor

@umarcor umarcor commented Nov 29, 2020

This PR is not to be merged per se, but for showcasing a problem.

In the context of hdl/MINGW-packages, I'm writing a PKGBUILD file for having iverilog upstreamed as an MSYS2 package (packages.msys2.org). See hdl/MINGW-packages@iverilog: mingw-w64-iverilog.

So far, make, make check and make install run successfully. Unfortunately, when testing the resulting package, I get Segmentation fault on MINGW64 (it passes on MINGW32).

This PR contains a PKGBUILD similar to the one in hdl/MINGW-packages, but here sources are not retrieved, since the build is executed in subdir msys2 (new, where PKGBUILD is located). Furthermore, a GitHub Actions workflow is added. It uses Action setup-msys2, mimicking the workflow in the upstream msys2/MINGW-packages.

NOTE: the simple test is a random repo I found in GitHub.

Friendly nudge to some users who have been recently dealing with building and using iverilog on Windows: @martinwhitaker @ktbarrett @garmin-mjames

@martinwhitaker
Copy link
Collaborator

The ivtest suite passes when I run it locally using MSYS2 with both 32-bit and 64-bit builds. CI tests a 64-bit MSYS2 build, which also generally passes (there is an occasional failure, usually just one test, but a different one every time, which I can't reproduce locally). See the .travis.yml file for details of how the CI tests are run and https://travis-ci.com/github/steveicarus/iverilog/branches for the CI results.

Note that the CI failures on the v11 branch are just due to the CI only being set up for checking the master branch.

@umarcor
Copy link
Contributor Author

umarcor commented Nov 29, 2020

@martinwhitaker, thanks for pointing to the CI service. I had overlooked that, since it is not shown for PRs.

What branch/release do you suggest to use for packaging?

@martinwhitaker
Copy link
Collaborator

CI is normally shown for PRs, but it seems we have run out of CI credits...

New development should always be done against the master branch. Changes that don't break API or ABI compatibility can then be back-ported to the stable branch, which is what you'd want for packaging.

@steveicarus
Copy link
Owner

Looks like I created a bunch of conflicts for you by merging PR#400. Sorry!

@umarcor
Copy link
Contributor Author

umarcor commented Nov 30, 2020

@steveicarus, no worries at all. I expected that. This is the typical conflict which is hardly impossible to understand for machines but pretty simple (C&P) for humans 😄

@umarcor umarcor force-pushed the ci/msys2 branch 2 times, most recently from 6732672 to 7292bfc Compare November 30, 2020 01:47
@umarcor umarcor marked this pull request as draft November 30, 2020 04:08
@umarcor umarcor mentioned this pull request Nov 30, 2020
@umarcor umarcor force-pushed the ci/msys2 branch 3 times, most recently from 1e1883d to 8e9192c Compare November 30, 2020 23:51
@umarcor umarcor mentioned this pull request Dec 1, 2020
4 tasks
@umarcor
Copy link
Contributor Author

umarcor commented Dec 1, 2020

This WIP is now based on #404, and it shows 12 different setups:

  • Two entrypoints: mingw64 (the one used in this repo's Travis) or msys2 (the default provided by setup-msys2).
  • Two build procedures: manual (the steps used in this repo's Travis) or pkgbuild (running makepkg-mingw and pacman -U).
  • Three environments: 🧊 (built-in MSYS2 installation) or 🟪 MINGW64|🟪 MINGW64 (clean MSYS2 installations).

Conclusions so far:

🧊 mingw64 🟪 MINGW64 mingw64 🟪 MINGW32 mingw64 🧊 msys2 🟪 MINGW64 msys2 🟪 MINGW32 msys2
manual OK OK OK OK OK 3 segfaults
pkgbuild many segfaults many segfaults - many segfaults many segfaults 3 segfaults

@umarcor
Copy link
Contributor Author

umarcor commented Dec 2, 2020

I asked MSYS2 maintainers (December 1, 2020 5:51 PM) and I was told:

we enable ASLR in PKGBUILDs.. maybe it exposes bugs in iverilog
you could set LDFLAGS="-pipe" in the PKGBUILD to disable it

I tried that, and it got rid of the inconsistencies between 'manual' and 'pkgbuild' procedures. The current set of tests is the following:

🧊 🟪 MINGW64 🟪 MINGW32
mingw64 · manual OK OK OK
msys2 · pkgbuild OK OK 3 segfaults

My understanding is that the mingw64 entrypoint forces the shell to be of type MINGW64, because of option -mingw64 (https://github.com/steveicarus/iverilog/blob/master/.github/workflows/test.yml#L62). Therefore, results of case mingw64 · manual · MINGW32 are incorrect, since there is no MINGW32 build in fact. I am inclined to think that iverilog has not been regularly tested on Windows 32 bits; hence, those three segfaults might be legit bugs. @steveicarus, can you please confirm? The specific failing tests are the following:

@martinwhitaker
Copy link
Collaborator

None of the Icarus developers normally use Windows. I check that it builds and passes the test suite with MSYS2 (using both a mingw32 and a mingw64 shell) before each release, and occasionally in between. Testing now, it does appear that a recent MSYS2 update has broken those 3 tests for mingw32.

@umarcor
Copy link
Contributor Author

umarcor commented Dec 2, 2020

Thanks for clarifying. I was worried I might have broken something because some days ago MINGW32 did pass...

Hopefully, after those 3 tests are fixed, finishing this PR will allow MSYS2 testing to be automated both on MINGW32 and MINGW64, for each push/PR. That should allow you to test manually before releases only.

Any thoughts regarding the comment about ASLR maybe exposing bugs in iverilog? I'm ok with a confirmation that it cannot be supported and it needs to be disabled.

@umarcor umarcor changed the title Segmentation fault on MINGW64 (MSYS2 packaging) CI: test on MINGW64 and MINGW32, use PKGBUILD recipe Dec 3, 2020
@umarcor
Copy link
Contributor Author

umarcor commented Dec 3, 2020

After @martinwhitaker's fixes to master (thanks!), this PR is ready.

@umarcor umarcor marked this pull request as ready for review December 3, 2020 08:38
@steveicarus
Copy link
Owner

Looks good.

I think we need documentation, somewhere, of the msys2 packaging and how to use it. The iverilog.wikia.com documentation site, and/or the iverilog.icarus.com main site?

Copy link
Owner

@steveicarus steveicarus left a comment

Choose a reason for hiding this comment

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

You've put a lot of effort into this. Thank you! It would be great if you can write up some documentation, possibly a README in the source tree and/or documentation in the iverilog.wikia.com documentation site, showing people how to put the msys2 stuff to good use. It would be a shame if all this effort goes unused for lack of documentation.

Also, double checking, you're ready for this to be merged?

@martinwhitaker
Copy link
Collaborator

Feel free to update https://iverilog.fandom.com/wiki/Installation_using_MSYS2.
If you get the binary package upstreamed, add a section to https://iverilog.fandom.com/wiki/Installation_Guide#Installation_From_Premade_Packages.

(wikia.com redirects to fandom.com)

BTW, I can't think of a good reason why ASLR should cause failures, so we should investigate that.

@steveicarus steveicarus merged commit df72e1d into steveicarus:master Dec 3, 2020
@umarcor umarcor deleted the ci/msys2 branch December 4, 2020 00:12
@umarcor umarcor mentioned this pull request Dec 4, 2020
@umarcor
Copy link
Contributor Author

umarcor commented Dec 4, 2020

Also, double checking, you're ready for this to be merged?

I wanted to submit the readme (#406), but a I had a long meeting. I'm sorry I didn't make it in time for this merge.
Anyway, now the wiki is updated and we can discuss the content of the README in #406.

If you get the binary package upstreamed, add a section to iverilog.fandom.com/wiki/Installation_Guide#Installation_From_Premade_Packages.

I will. Nonetheless, as mentioned in the readme proposed in #406, there are pre-made packages available already. Each CI run generates two artifacts (one for MINGW32, another one for MINGW64) which users can download, unzip and install with pacman -U *.zst.

It is unfortunate that GitHub wraps the tarballs in zipfiles. However, there is a solution that you might want to use. In ghdl/ghdl, artifacts are pushed to a prerelease named 'nightly':

Copying those 14 lines to the existing workflow would provide the same feature in this repo.

I understand that the section of the wiki is rather meant for "official" repositories, and it seems that iverilog will be upstreamed to MSYS2 soon. That's why I didn't add this content.

BTW, I can't think of a good reason why ASLR should cause failures, so we should investigate that.

Indeed... A maintainer of MSYS2 said: "ASLR will likely be enabled by default for the next binutils release (not sure when)" (December 2, 2020 12:35 PM). When that change is done, building iverilog using the manual procedure explained in the wiki will fail. So, it's not a problem yet, but we should either find out which the problem is, or suggest that LDFLAGS="-pipe" is used for disabling ASLR.

@martinwhitaker
Copy link
Collaborator

The bug that was causing failures with ASLR enabled is now fixed. I have removed the LDFLAGS="-pipe" from the MSYS2 PKGBUILD.

@umarcor
Copy link
Contributor Author

umarcor commented Dec 5, 2020

Awesome! Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants