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

Use develop in tests #469

Closed
ISSOtm opened this issue Jan 13, 2020 · 29 comments
Closed

Use develop in tests #469

ISSOtm opened this issue Jan 13, 2020 · 29 comments
Labels
enhancement Typically new features; lesser priority than bugs tests This affects the test suite

Comments

@ISSOtm
Copy link
Member

ISSOtm commented Jan 13, 2020

[EDIT] This is the current state of the issue; the original post can be found unedited below the horizontal separator.

CI has been moved to GitHub Actions, and develop is enabled in exactly one of the build jobs.

What is needed? Looking into ways to enable develop for as many jobs as possible. Our Makefile is currently POSIX-compatible, but there's been talk to drop that and switch to GNU Make only, so it's not excluded.


(Half of this overlaps with #283.)

After b49e025 accidentally broke error reporting (+ undefined behavior, etc etc 🤷‍♂️), it might be a good idea to run make develop instead of make in the CI.
I've done that in the develop branch... and the five jobs all fail.

Clang

Clang (9.0.1 on my machine) complains about the following flags:

  • -Wformat-overflow=2
  • -Wformat-truncation=1
  • -Wstringop-overflow=4
  • -Walloc-zero
  • -Wduplicated-cond
  • -Wlogical-op
  • -Wno-aggressive-loop-optimizations

GCC

The GCC version Travis uses by default is 5.4.0, which fails to recognize the following flags:

  • -Walloc-zero
  • -Wduplicated-cond
  • -Wformat-overflow=2
  • -Wformat-truncation=1
  • -Wstringop-overflow=4

However, GCC 7.4.0 is available in Travis, and I have confirmed on my machine that 7.5.0 works correctly, but links against libubsan.so which I don't have installed. GCC 6.4.0 fails to recognize the options as well.


It seems possible to require a minimum GCC version with Travis, as mentioned in the link above, but that seems to be—bluntly—a clusterfuck. I'm considering trying to move to GitHub Actions instead (which I would try to set up in a separate repo of mine first, of course). That would have the added benefit of Windows testing.

Thoughts on the following?

  • Moving from Travis to Actions
  • Dropping GCC < 8.x for development and CI
  • Different configurations (testing, CI, releases) [would require GNU Make, probably, but it's already a goal according to the discussion in Track C dependencies automatically #428]
@ISSOtm ISSOtm added enhancement Typically new features; lesser priority than bugs tests This affects the test suite labels Jan 13, 2020
@AntonioND
Copy link
Member

Yeah, make develop pretty much only works if you have the exact version of the compiler that understands all the warning options. But it's not like there is an option to enable all warnings. All the options to enable all warnings are a big lie.

@mid-kid
Copy link
Contributor

mid-kid commented Jan 26, 2020

It's not wrong to require a specific compiler when you're gonna enable all sorts of instrumentation, especially when some of it is more contemporary, so I wouldn't bother making this "portable", as long as it isn't the default.
That said, I haven't found many cases where -Wall -Wextra didn't cover my needs so you should consider whether all of the flags you're enabling are within reason - there's a reason they're not default.
And yeah you're going to have to make sure your compiler has been built with the sanitizers enabled for them to work :P

@aaaaaa123456789
Copy link
Member

If you're going to rely on CI infrastructure with a specific compiler available, it seems OK to depend on that compiler to run the tests, assuming the compiler itself can be easily obtained.

@ISSOtm
Copy link
Member Author

ISSOtm commented Jan 26, 2020

The problem with relying on a single compiler is that we have already had one compiler catch errors the other didn't (I personally had Clang catch errors GCC let go, but GCC is the one supporting the flags in question). That's why I would prefer to find a way that lets us keep both compilers in the CI.

@aaaaaa123456789
Copy link
Member

You can set up two CI tasks with specific compilers and specific flags for each.

@ISSOtm
Copy link
Member Author

ISSOtm commented Jan 27, 2020

I had in mind to have develop produce different compiler flags for GCC and Clang (using GNU Make, WIP locally) but I'm not sure how to detect the current C compiler (such as when using cc)

@aaaaaa123456789
Copy link
Member

Set the flags externally in the CI script along with the compiler itself (i.e., set both CC and CFLAGS).

@mid-kid
Copy link
Contributor

mid-kid commented Jan 27, 2020

That, or wrap it in a shell script you can run outside of CI, or have develop_gcc and develop_clang. Keep it simple.

@ISSOtm
Copy link
Member Author

ISSOtm commented Jan 28, 2020

I wanted to separate the target compiler (including MinGW) from the target configuration (debug / release).

@ISSOtm
Copy link
Member Author

ISSOtm commented Jan 31, 2020

I have set up a test repo to test Actions on. Testing the *nix builds works like a charm, but trying to get the Windows builds is a nightmare.

The main binaries fail to compile because there are no libpng development headers for mingw (could be worked around by using the base system's given that they are basically identical, but that sounds hacky to me).

zlib1.dll is successfully compiled, but we tried three different ways to compile libpng, to no avail. CMake generates no Makefile (???), the GCC-generic Makefile can only produce a static lib (being generic), and the Windows-specific Makefile has no target to produce a DLL. I'm quite at a loss as to what to do with this.

@AntonioND
Copy link
Member

Yeah, it is a nightmare, and that's why I decided to cross compile the Windows binaries...

@ISSOtm
Copy link
Member Author

ISSOtm commented Jan 31, 2020

Well, this is cross-compiling the binaries (just not the libs), but there are missing headers. On my Arch box, I have those headers in a package so the current solution (MinGW's pkg-config) works just fine; how do you do it yourself?

@AntonioND
Copy link
Member

I have a Fedora VM that has libpng and zlib as packages to cross compile for Windows. I don't think that Debian has them, for example. It took me a couple of tries to find a distro that had them.

@ISSOtm
Copy link
Member Author

ISSOtm commented Jan 31, 2020

I currently have two ideas:

  • Make mingw's pkg-config read the host system's headers (they're identical enough on my machine at least, but it's a hack and I don't like it)
  • Manually insert the headers + pkg-config config files in the appropriate directories; this I cannot debug due to lacking a machine (I could set up a VM but that'd take time) and debugging a CI box is annoying as all hell.

Other option, pray that macOS can do the job. (I doubt it.)

@mid-kid
Copy link
Contributor

mid-kid commented Jan 31, 2020

CMake generates no Makefile

Have you tried -G 'Unix Makefiles'?
More info: https://cmake.org/cmake/help/latest/manual/cmake-generators.7.html

@ISSOtm
Copy link
Member Author

ISSOtm commented Feb 13, 2020

Finally did it..!

For the binaries, I eventually just went "fuck it" and mimicked Arch Linux's build script for libpng headers. Works wonders, we have the binaries now—this requires, however, having copies of two MinGW scripts in the repo, especially as one of the scripts is broken and does not run on the build machine by default.

For the DLLs, I realized by luck that the MinGW package (for zlib) and compilation (for libpng) yield DLLs at /usr/<triplet>/lib/zlib1.dll and /usr/<triplet>/bin/libpng16-16.dll, so this removes the separate DLL build step, thankfully.

Currently, however, the test suite fails on Windows... blame rgbgfx somehow. Fixing this would require a Windows VM though, which I currently do not have. (Note: the binaries are available in the tiny "Artifacts" link near the top.)
[EDIT] Just noticed the 64-bit test run does not error out at this point... GitHub aborted the run once the 32-bit one failed, so we don't know whether the 64-bit build actually passes.
[EDIT2] Disabling the 32-bit build let the 64-bit one actually finish, and it passes!

@ISSOtm
Copy link
Member Author

ISSOtm commented Feb 14, 2020

It was determined that the 32-bit build links against libgcc_s_sjlj-1.dll, whereas the 64-bit one does not. Not exactly sure what to do... It was suggested to link libpng with -static-libgcc, but libtool ignores the flag for some reason? Other possibility, do build the DLLs on Windows, apparently it might be possible with CMake -G 'NMake Makefiles'.

@ISSOtm
Copy link
Member Author

ISSOtm commented Feb 14, 2020

I was able to enable make develop on Ubuntu 18.04 with GCC, but that's about it. It's a start, I suppose—we'd just need people to use that one by default. (I cheated slightly and got it to be the first of the *nix builds in the list.)

@ISSOtm
Copy link
Member Author

ISSOtm commented Feb 14, 2020

After more testing, even the 32-bit MSVC build of libpng just fails.

I think we should just deprecate (by distributing the 32-bit bundle and telling people to get the GCC runtime somehow) or outright drop 32-bit support. What do you say?

@AntonioND
Copy link
Member

I think you can remove it. If you need the 32-bit windows build you need to:

  • Be interested in GB development.
  • Want to develop in an old 32-bit PC rather than a more modern one.
  • Have an old 32-bit PC that doesn't run Linux and just runs Windows XP or something.

This is pretty unlikely...

@mid-kid
Copy link
Contributor

mid-kid commented Feb 15, 2020

It's more likely that you think, given how many people don't know how to install a new OS, and for the longest time a lot of windows 7 machines shipped with a 32-bit OS (despite their hardware being 64-bit capable). These sort of machines are still in use today and pop up every once in a bit in the pret discord. I don't want to turn these people away from hacking on a 20-y/o game on a machine that's more than capable of doing that.

If you guys don't want to support 32-bit windows I understand. I can always look into this myself or tell people to build it on cygwin.

@pinobatch
Copy link
Member

I imagine a lot of Windows Vista and Windows 7 machines had a 32-bit OS for a couple reasons:

  • Peripherals with no 64-bit driver. At one point, this was especially common in hardware hobbyist circles because of the cost of a kernel mode code signing (KMCS) certificate, which the x86-64 version of Windows requires in order to avoid a "Test Mode" banner in all four corners of the screen.
  • PCs with 2 GB or less RAM. A 64-bit Windows system needs to keep both x86 and x86-64 versions of system libraries loaded. And though x86-64 has more CPU registers, the additional data cache pressure from pointers being twice as big may outweigh this for some workloads. Consider that Dell was still shipping compact laptops with 2 GB of RAM until 2018, when 4 GB became the standard.

Would it help to take a survey on pret, gbdev, etc. to see what architecture everybody uses? How would a survey even work? Is it straightforward for someone who knows Game Boy programming but not obscurities of OS administration to determine whether Windows or Linux is 64-bit? I know for Linux you could ask people to do this:

  1. Open a terminal
  2. file -L /bin/sh
    (The -L causes file to follow symbolic links, which is the default in POSIX but not in GNU.)
  3. See whether 64-bit and/or x86-64 appear

What would the analogous procedure for Windows 10, 8.1, and 7?

@madpew
Copy link

madpew commented Feb 15, 2020

For windows machines the following should give the correct result on all platforms:
wmic os get OSArchitecture

However, I doubt basing this decision on the distribution in the current userbase is the right thing to do. Dropping 32bit-support (for now) because of the issue mentioned above should be alright.
If someone wants to jump on this and fix the build that should also be an option.

@aaaaaa123456789
Copy link
Member

Doesn't Windows also show the bitness under System in the Control Panel?

@madpew
Copy link

madpew commented Feb 15, 2020

Doesn't Windows also show the bitness under System in the Control Panel?

Yes, it does show on the System-screen as well (atleast on Windows 7) accessible by the shortcut WIN+PAUSE however I don't know if this works on all platforms.

@ISSOtm
Copy link
Member Author

ISSOtm commented Feb 18, 2020

To attain a compromise, we have this setup that fails on Win32, but skips the tests involving RGBGFX on it.
That's as good as I can do, and people can fairly trivially re-enable full Win32 testing in forks if they want to experiment.

@ISSOtm
Copy link
Member Author

ISSOtm commented Mar 6, 2020

Patch note regarding the previous comment: the Win32 testing has been re-enabled after some discussion with Windows users—the required GCC runtime is now packaged inside the 32-bit Windows zip.

@ISSOtm
Copy link
Member Author

ISSOtm commented Mar 6, 2020

Here's the current state of this: develop is enabled in one (ubuntu-18.04,gcc) of the CI's 6 tests (matrix of ubuntu-18.04,ubuntu-16.04,maos-10.15 ; gcc,clang). This should be sufficient to catch problems reported by ubsan (any output from it fails a test), but I'd like to have at least one Clang develop build. (That'd probably allow using develop on macOS at all, too.)

Opinions on what to do? On how to do it best?

@ISSOtm
Copy link
Member Author

ISSOtm commented Mar 10, 2022

develop has been in use in all *nix CI jobs since cf19879

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Typically new features; lesser priority than bugs tests This affects the test suite
Projects
None yet
Development

No branches or pull requests

6 participants