Novum / vkQuake

Vulkan Quake port based on QuakeSpasm
GNU General Public License v2.0
1.81k stars 220 forks source link

MINGW/MSYS2 : Fetch binary dependencies from the build environnement itself instead of relying on binaries kept in Git #703

Closed vsonnier closed 3 months ago

vsonnier commented 3 months ago

A follow-up of https://github.com/Novum/vkQuake/commit/7cf09be32046c812ad94e45a9eabcc84587a6188 :

We added libwinpthread-1.dll as a binary dependency for MSYS2/MINGW and stored a given version (MSYS2) in /Windows/misc/[x64|x86], in the same way we keep SDL2 and audio codec binaries.

As @j4reporting reported (pun intended) this is not very clean because this dll may have an adherence on the undelaying Mingw/MSYS2 runtime, even built static, so strictly speaking is not "portable" from one build to another.

An improvement would be to fetch 'libwinpthread-1.dll' from the current Mingw distribution used in Makefile.w[32|64] to garantee conststency.

vsonnier commented 3 months ago

@j4reporting Using $MINGW_PREFIX to find the installation looks OK, including for the CI. Is it good on your side ?

sezero commented 3 months ago

gcc in a mingw-w64 toolchain can be configured to not rely on winpthreads (e.g. my own linux-to-windows mingw-w64 cross-toolchains are as such), therefore, the unconditional copying of it in the makefiles can result in failures.

j4reporting commented 3 months ago

@vsonnier I now realize that the copy commands are in the Makefiles. This will probably break other mingw installations especially those without posix threads enabled or configured with --disable-shared like w64devkit It is necessary to check whether this dll exists.

I don't know if this could be moved to the build-mingw.yml workflow file.

@sezero mingw compilers distributed by the msys2 project, w64devkit, or provided by fedora and probably others are using posix threads.

sezero commented 3 months ago

@sezero mingw compilers distributed by the msys2 project, w64devkit, or provided by fedora and probably others are using posix threads.

That I already know. But what they provide isn't the universal truth, that's what I'm saying.

j4reporting commented 3 months ago

That I already know. But what they provide isn't the universal truth, that's what I'm saying.

I have never claimed that. I assume this MSYS2 environment is not custom built, but provided by github? Are there any disadvantages by using posix threads?

My prefered method would be to link the static library instead.
MSYS2 defines $MSYSTEM so just use this specific case for github's mingw workflow see https://github.com/j4reporting/vkQuake-vso/commit/d79821a392579da612d678eae465a6f609ff25b8 ignore the second part of the commit.

If you want to link the shared library then something like this should work, I would not use MINGW_PREFIX though. Make it specific for MSYS2 for now.

- MINGW_PATH = ${MINGW_PREFIX}
+LIBWINPTHREAD = $(MSYSTEM_PREFIX)/bin/libwinpthread-1.dll

dll:
    if [ -f $(LIBWINPTHREAD) ]; then \
        cp $(LIBWINPTHREAD)  . ; \
    fi
sezero commented 3 months ago

Are there any disadvantages by using posix threads?

Unnecessary dependence to winpthreads dll itself is a disadvantage, imo.

  if [ -f $(LIBWINPTHREAD) ]; then \
      cp $(LIBWINPTHREAD)  . ; \
  fi

Existence check looks ok to me

vsonnier commented 3 months ago

I've commited @j4reporting suggestion:

If you want to link the shared library then something like this should work, I would not use MINGW_PREFIX though. Make it specific for MSYS2 for now.

- MINGW_PATH = ${MINGW_PREFIX}
+LIBWINPTHREAD = $(MSYSTEM_PREFIX)/bin/libwinpthread-1.dll

dll:
    if [ -f $(LIBWINPTHREAD) ]; then \
        cp $(LIBWINPTHREAD)  . ; \
    fi

For now, this is Good Enough (TM) for me. Feel free to improve later if you feel like it.

Thanks for your help !