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

Update to compilation workflows #46830

Merged
merged 2 commits into from
Feb 6, 2021

Conversation

ToxiClay
Copy link
Contributor

@ToxiClay ToxiClay commented Jan 17, 2021

Summary

Infrastructure "Refine/revise workflow for Windows compilation targets"

Purpose of change

This PR addresses the need for revisions and refinements to CDDA's compilation workflows for at least three environments:

  1. Cross-compilation under Linux
  2. Native compilation under Windows using MSYS2
  3. Native compilation under Windows using Cygwin

Describe the solution

Using virtual machines, I stepped through each of the three workflows, documenting steps as I went, and relying heavily on Discord support.

The workflow for native Windows compilation using MSVS, however, has a fatal error due to a busted dependency on yasm-tool which prevents its installation, meaning further toolchain building is currently impossible.

Describe alternatives you've considered

Waiting for someone else to do it.

Testing

I successfully compiled and executed Cataclysm in each of the three environments I tested.

Additional context

@lopho
Copy link
Contributor

lopho commented Jan 17, 2021

This comes just in time for me, as I try to get cmake up to speed. Appreciated, great resource.

@BrettDong BrettDong added <Documentation> Design documents, internal info, guides and help. [Markdown] Markdown issues and PRs labels Jan 17, 2021
README.md Outdated
@@ -45,7 +45,8 @@ Please read [COMPILING.md](doc/COMPILING/COMPILING.md) - it covers general infor

We also have the following build guides:
* Building on Windows with `MSYS2` at [COMPILING-MSYS.md](doc/COMPILING/COMPILING-MSYS.md)
* Building on Windows with `vcpkg` at [COMPILING-VS-VCPKG.md](doc/COMPILING/COMPILING-VS-VCPKG.md)
<!-- Instructions temporarily deprecated due to fatal error, tracked as issue 15087 at https://github.com/microsoft/vcpkg/ -->
Copy link
Contributor

Choose a reason for hiding this comment

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

There could be all kinds of errors in third party tools (e.g. in vcpkg), but there is no need to hide/remove instruction which is perfectly fine because of any possible errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it "perfectly fine" if the instructions don't work, though?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, our instructions are perfectly fine - they worked before the issue hit vcpkg and they will work after that vcpkg issue is resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not against updating the instruction, but it would be better to change it in such generic way that we won't need to change it again once a different issue hits vcpkg - e.g. refer to specific version of vcpkg that does not have any known issues when building CDDA, totally ignore any issues that aren't directly related to CDDA (and assume there are would be no error when installing vcpkg, building CDDA), assume there are would be all kinds of errors and ask reader to seek help for possible solutions in vcpkg repo - something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally ignore any issues that aren't directly related to CDDA (and assume there are would be no error when installing vcpkg, building CDDA)

So, while I understand that we can't possibly keep our instructions up-to-date with every single change, I ran into this error while attempting to clean them up, and it's a hard stop. I don't know if there is a version of the yasm subcomponent that works -- if there is, I sure couldn't find it, and knowing that something is fatally wrong, leaving the instructions as they are doesn't exactly sit well with me.

I admit I don't have a perfect solution, but "ignore the error and assume everything works" doesn't feel right -- not least of which because I can't verify the downstream instructions work.


2. Run downloaded file and install MSYS2 (click `Next` button, specify directory where MSYS2 64 bit will be installed (e.g. `C:\msys64`), click `Next` button again, specify Start Menu folder name and click `Install` button).
2. Run the installer. It is suggested that you install to a dev-specific folder (C:\dev\msys64\ or similar), but it's not strictly necessary.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it is suggested? MSYS 2 installer suggests a different path and I see no value in changing it to a longer path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recommend this so that the user has the compiler and the code being compiled logically near each other on the drive, so that way they're not bouncing around their drive like a ping-pong ball. VCPKG's homepage even recommends this.

First, download and bootstrap vcpkg itself; it can be installed anywhere, but generally we recommend using vcpkg as a submodule for CMake projects, and installing it globally for Visual Studio projects. We recommend somewhere like C:\src\vcpkg or C:\dev\vcpkg, since otherwise you may run into path issues for some port build systems.

Copy link
Contributor

Choose a reason for hiding this comment

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

We recommend somewhere like C:\src\vcpkg or C:\dev\vcpkg, since otherwise you may run into path issues for some port build systems.

I think it just means: use a short path without spaces.

```

2. If asked close MSYS2 window and restart it from Start Menu or `C:\msys64\msys2_shell.cmd`.
2. MSYS will inform you of a cygheap base mismatch and inform you a forked process died unexpectedly; these errors appear to be due to the nature of `pacman`'s upgrades and *may be safely ignored.* You will be prompted to close the terminal window; do so, then re-start using the MSYS2 MinGW 64-bit menu item.
Copy link
Contributor

Choose a reason for hiding this comment

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

Different versions of third party tools that can report different errors at different time. These errors aren't CDDA-specific and should not be addressed in CDDA repo.

Copy link
Contributor Author

@ToxiClay ToxiClay Jan 17, 2021

Choose a reason for hiding this comment

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

They're not CDDA-specific, no, but they are MSYS2-specific, and Eso told me I should call it out.

[4:40 PM] esotericist: if it turns out the error is due to your step, but is harmless, you should still document it because odds are good someone else might do that innocently

The error, as it turns out, is not due to a step I took, but will always happen when you follow the directions we give, and so it should be noted.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is noted on MSYS2 site iirc.

Copy link
Contributor

@actual-nh actual-nh Jan 17, 2021

Choose a reason for hiding this comment

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

Even if something isn't CDDA-specific, mentioning said errors may help divert people from complaining about them in CDDA fora (issues, discord...). OTOH, one can go too far in this - I can see both arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is noted on MSYS2 site iirc.

Sure, but users would only be on MSYS2's website to download the installer. They're following our instructions for compilation, and as NH says, it's our due diligence.

@ZhilkinSerg ZhilkinSerg added Code: Build Issues regarding different builds and build environments OS: Windows Issues related to Windows operating system labels Jan 17, 2021
PLATFORM="i686-w64-mingw32.static"
make CROSS="~/src/mxe/usr/bin/${PLATFORM}-" RELEASE=1 LOCALIZE=1

<!-- Building ncurses for Windows is a nonstarter, so the directions were removed. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to build the ncurses version on Windows with MSYS2, so it is still possible if the cross toolchain has the suitable libs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's good to know, I suppose. However, per Kevin:

[3:12 AM] ToxiClay: Is it OK if I strip ncurses out of compilation instructions for Windows, as we won't be compiling ncurses builds on or for Windows? Like, the Cygwin instructions call for apt-cyg install libncurses-devel, and I'm pretty sure I can remove it with no problem

[3:14 AM] kevingranade: yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm kinda surprised you were able to build it; I was under the impression Windows ncurses simply didn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

It might work, but it's not at all supported, and it's subject to being ripped out by the roots.
The rationale for which is that on windows our curses replacement library has worse performance than the SDL build in text mode, and as far as I know no features are enabled by the terminal build on windows.
Specifically, on linux having a terminal build enables playing over ssh and easier integration with some screen readers, I'm not aware of any such use cases on windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

By ncurses version I mean the original linux ncurses ported to Windows, provided by MSYS2, not our curses 'port' implemented using the Windows API. I managed to build it when I was doing UI fixes and wanted to test if it worked in the ncurses version but didn't have a linux machine. I imagine with the proper ported libs and tools, playing over ssh and with screen readers could be possible. That said, I don't think anyone would want to build on Windows with ncurses apart from testing, so it's probably fine not to mention it in the docs.

Copy link
Member

Choose a reason for hiding this comment

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

oh wow, I had absolutely no idea that was functional.

-Ubuntu 20.04, native and cross-compilation
-Windows, compilation using MSYS2, Cygwin, and Visual Studio
@kevingranade kevingranade merged commit 6582277 into CleverRaven:master Feb 6, 2021
feinorgh pushed a commit to feinorgh/Cataclysm-DDA that referenced this pull request Feb 8, 2021
* Update and confirm compilation workflows for various environments:

-Ubuntu 20.04, native and cross-compilation
-Windows, compilation using MSYS2, Cygwin, and Visual Studio

* Include warning message about whitespace
@Pupsi-Mupsi
Copy link
Contributor

@ToxiClay

Are you sure that vs compiles with sound?

This will configure Visual Studio to compile the release version, with support for Sound, Tiles, and Localization (note, however, that language files themselves are not automatically compiled; this will be done later).

@ToxiClay
Copy link
Contributor Author

@ToxiClay

Are you sure that vs compiles with sound?

This will configure Visual Studio to compile the release version, with support for Sound, Tiles, and Localization (note, however, that language files themselves are not automatically compiled; this will be done later).

I don't see why it wouldn't do so. That was the impression I had.

@ZhilkinSerg
Copy link
Contributor

There SDL_SOUND compilation flag defined in project settings. It is building with sound support.

@Pupsi-Mupsi
Copy link
Contributor

Thanks for the confirmation. I recently installed VS2019 anew following the instruction (https://github.com/CleverRaven/Cataclysm-DDA/blob/master/doc/COMPILING/COMPILING-VS-VCPKG.md - 64-bit dependencies only). The game compiles and everything but I don't have any sound. Honestly I don't remember that I ever had any. Thats why I asked in the first place. What did I do wrong?

@ZhilkinSerg
Copy link
Contributor

Incompatible file format in soundpack probably.

@Pupsi-Mupsi
Copy link
Contributor

Indeed you are completely right. I tested the "basic soundpack" after converting the menu_move.ogg sound to menu_move.wav and everything works just fine. Seems that OGG is not supported then.

@olanti-p
Copy link
Contributor

olanti-p commented Aug 6, 2021

@ZhilkinSerg Multiple people have also reported lack of sound in VS builds of BN, and I had it with DDA on experimental 0.F.

After some digging and looking at debug.log it turned out that sdl2_mixer needs vorbis plugin to load basic soundpack's .oggs, and that plugin was supposed to be loaded from DLL due to enabled dynamic-load feature. But neither VS nor vcpkg copy over the DLLs, so the game was spamming something along the lines of "Failed to load sound: Failed loading vorbisfile.dll".

Solved it for BN by statically linking sdl2_mixer's plugins and including shlwapi.lib for some missing symbols from mpg123.
cataclysmbnteam/Cataclysm-BN@2cb8123

@ZhilkinSerg
Copy link
Contributor

Thanks.

wapcaplet added a commit to wapcaplet/Cataclysm-DDA that referenced this pull request Feb 4, 2022
This heading was removed (probably by accident) in CleverRaven#46830.
kevingranade pushed a commit that referenced this pull request Feb 5, 2022
* Restore "Building with Visual Studio" heading

This heading was removed (probably by accident) in #46830.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code: Build Issues regarding different builds and build environments <Documentation> Design documents, internal info, guides and help. [Markdown] Markdown issues and PRs OS: Windows Issues related to Windows operating system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants