Add MSVC_MP CMake option to adjust number of parallel build jobs

[SunBlack]

No description provided.

[SergioRAgostinho]

Two Three comments here:

• The logic is a little convoluted
• I'm on a long term effort to stop us from writing on CMAKE_*_FLAGS
• Are you sure you can remove that version check on MSVC?

Here's my suggestion

if(USE_MSVC_MP EQUAL 0)
  # USE_MSVC_MP is 0 in case the information cannot be determined by ProcessorCount => fallback
  add_compile_options("/MP")
elseif(USE_MSVC_MP GREATER 1)
  add_compile_options("/MP${USE_MSVC_MP}")
endif()

Assuming I'm not mistaken, this code should be equivalent.

[SunBlack]

Are you sure you can remove that version check on MSVC

See #2690 (comment). Since C++14 is now necessary we require at least VS 2015 (MSVC_VERSION 1900) now.

I will check you suggestion. Looks indeed better :).

[SunBlack]

Tested it: And it works :).

Side node: MSVC seems to ignore passed process count and only differ between with and without /MP. But I'm not sure if it is a UI error of VS or not. But documentation says we can pass processor count...

[SergioRAgostinho]

After reading the documentation I noticed that one can pass /MP1 which makes the elseif() useless.

Also I'm questioning if USE_MSVC_MP is the best name to use here, since it gives the idea that is a binary var from the name. Maybe remove the prefix USE_?

Finally, our CI might blow-up out of memory if we don't tweak the number of compilations processes running, but we can tweak that later.

[SunBlack]

After reading the documentation I noticed that one can pass /MP1 which makes the elseif() useless.

I was thinking about it, too. But I there was a decision: Add a useless flag (MP1) or add two more CMake lines and prevent useless flag ;). And I didn't tried what happens in case someone enters -1 (if it will be ignored then or cause a compiler issue)

Also I'm questioning if USE_MSVC_MP is the best name to use here, since it gives the idea that is a binary var from the name. Maybe remove the prefix USE_?

I added USE_ before so it is in same group like USE_PROJECT_FOLDERS. But we could also start a group MSVC.

Finally, our CI might blow-up out of memory if we don't tweak the number of compilations processes running, but we can tweak that later.

I didn't changed behavior to the state of pre-PR.

[SergioRAgostinho]

I was thinking about it, too. But I there was a decision: Add a useless flag (MP1) or add two more CMake lines and prevent useless flag ;). And I didn't tried what happens in case someone enters -1 (if it will be ignored then or cause a compiler issue)

Makes sense. With the current logic, it will be ignored. Edit: if the elseif() was not there it would throw a compiler error.

I added USE_ before so it is in same group like USE_PROJECT_FOLDERS. But we could also start a group MSVC.

In this case better go with the MSVC group. USE_blah should be binary in my opinion.

I didn't changed behavior to the state of pre-PR.

From the logic it seems that by default it will always try to use the number if gets from ProcessorCount(). Everything went ok, so it is not a problem.

[SunBlack]

I added USE_ before so it is in same group like USE_PROJECT_FOLDERS. But we could also start a group MSVC.

In this case better go with the MSVC group. USE_blah should be binary in my opinion.

What do you prefer as new name MSVC_MP, MSVC_simultaneously_compilers or whatever?

From the logic it seems that by default it will always try to use the number if gets from ProcessorCount(). Everything went ok, so it is not a problem.

As before ;)

[SergioRAgostinho]

MSVC_MP works for me. The docstring is pretty clear.

As before ;)

Oh, true.

[taketwo]

LGTM, but I'm not a MSVC user.

[UnaNancyOwen]

LGTM