Fixup MSVC source file inclusion for cmake builds

[hmaarrfk]

Related to: #2951

Working builds in conda-forge: conda-forge/zstd-feedstock#56

[Cyan4973]

It seems that, while this PR is related to #2951,
it's not meant to complement it,
but rather is in competition with it.

Both try to achieve similar objectives, altering the cmake build system,
though with differences :

• This PR is more permissive, as it allows the *.S file for anything other than MSVC
• meson: fix MSVC support #2951 is more restrictive, as it only allows *.S for either gcc or clang.

So any other compiler which is not part of this list will be either accepting the assembly file (first case) or not (second case). There is also the interesting corner case of clang on MSVC, where the decision process is reversed (refused for first case, accepted in second case).

[hmaarrfk]

I guess here I am altering the included cmake files. Is there an other way to regenerate these?

You are correct in identifying the nuances. If you can regenerate the cmake files, then I'm happy to close this one. It just seemed that the logic was different enough that the cmake code was hand written.

[Cyan4973]

Wait, I misinterpreted,
this PR is about cmake, while #2951 is about meson,
they both fix the same "problem", though differently, generating potentially different side effects.

So, essentially, same questions apply :

• Could we have a test in our CI that would be able to generate and therefore find the same issue
• Would it be possible, if not better, to have a solution at source code level, so that it works with all build systems, rather than fixing each build system one after another.

[hmaarrfk]

ok. it seems that you are using msbuild in your internal ci.

Honestly, it is alot of work to maintain multiple build systems. msbuild, meson, cmake, seems like a big build matrix. it can be done, but do you really want that?

it is also a little more than the amount of time i wanted to spend on this.

if you wanted to let us know the behaviour you want, we can make sure to implement it in the (hopefully) the way between meson and cmake. I know patches like this are reactionary, but often it is hard to anticipate all the different integration methods that downstream users will think of

[eli-schwartz]

So any other compiler which is not part of this list will be either accepting the assembly file (first case) or not (second case).

The correct choice is to reject it for all compilers other than those that are or identify as gcc/clang, because this source file is not C and you cannot assume any C compiler will understand it.

In fact, the compiler that understands it isn't, tech ucally, gcc either, it is gas... they are just both frontends to the same backend and gcc doesn't care which you use in this case.

There is also the interesting corner case of clang on MSVC, where the decision process is reversed (refused for first case, accepted in second case).

Aren't those both compilers? How do you have a compiler on a compiler?

The check should be matching a compiler (gcc, clang, MSVC), not a generator (unix makefiles, nmake, xcode, Visual Studio, ninja). Whichever compiler is being run to produce an intermediate *.o file needs to be one that understands the GNU dialect of assembly.

[eli-schwartz]

What I don't understand is...

cmake-visual-2019 (Visual Studio 16 2019, -A Win32)

This CI job uses cmake to generate a VS project with the detected compiler being MSVC.

So why did this CI job succeed before this PR?

When I submitted the meson PR my naive assumption was that maybe cmake automatically ignores that file because it knows MSVC cannot build asm.

[hmaarrfk]

I don't think all the tests are being run on master, at least it is hard for me to find them.

I created #2959 to see what the current test suite does.

[eli-schwartz]

Some commits seem to only be running 6 tests, others are running in the range of 70.

The important one is https://github.com/facebook/zstd/runs/4402057229?check_suite_focus=true which builds commit a74a369

At this point you can check that:

• cmake includes the .S file via glob: https://github.com/facebook/zstd/blob/a74a36985a7c296eba0ca0bb84f9bbee0f0dca0e/build/cmake/lib/CMakeLists.txt
• the .S file is present: https://github.com/facebook/zstd/tree/a74a36985a7c296eba0ca0bb84f9bbee0f0dca0e/lib/decompress
• the cmake build is running in CI, it generates a VS solution, the detected compiler is MSVC, and the .S file is NOT listed in the console output from msbuild, so I assume it is somehow not actually included. The run is successful.

[terrelln]

One thing to fix and this looks good to me.

[felixhandte]

Looks good to me!