visualstudio.py: Don't error out when removing /utf-8 (fix regression)

[fanc999]

Hi,

From the commit message:

In PR 10263, we didn't account for that we may initialize the Visual Studio-like compiler two times, once for a C compiler and once for the C++ compiler, so we end up with Meson breaking on Visual Studio 2013 or earlier, such as when building GLib.

Fix this by checking for a ValueError, just like what we did when /[source|execution]-charset:... is specified, , so that we do not error out when we have already removed /utf-8 from the set of always-used compiler arguments.

With blessings, thank you!

[codecov]

Codecov Report

Merging #10470 (b6a36e3) into master (c151988) will decrease coverage by 1.49%.
The diff coverage is n/a.

❗ Current head b6a36e3 differs from pull request most recent head e3664fb. Consider uploading reports for the commit e3664fb to get more accurate results

@@            Coverage Diff             @@
##           master   #10470      +/-   ##
==========================================
- Coverage   68.71%   67.22%   -1.50%     
==========================================
  Files         406      203     -203     
  Lines       87868    43911   -43957     
  Branches    19522     9768    -9754     
==========================================
- Hits        60379    29517   -30862     
+ Misses      22918    12139   -10779     
+ Partials     4571     2255    -2316     

Impacted Files	Coverage Δ
modules/unstable_cuda.py	0.00% <0.00%> (-72.50%)	⬇️
templates/cudatemplates.py	37.50% <0.00%> (-62.50%)	⬇️
compilers/cuda.py	19.38% <0.00%> (-45.54%)	⬇️
dependencies/cuda.py	20.19% <0.00%> (-42.79%)	⬇️
compilers/detect.py	41.11% <0.00%> (-5.04%)	⬇️
linkers/linkers.py	56.30% <0.00%> (-1.28%)	⬇️
environment.py	78.98% <0.00%> (-1.07%)	⬇️
dependencies/misc.py	44.44% <0.00%> (-0.75%)	⬇️
compilers/compilers.py	82.18% <0.00%> (-0.47%)	⬇️
mesonlib/universal.py	76.45% <0.00%> (-0.32%)	⬇️
... and 211 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c151988...e3664fb. Read the comment docs.

[eli-schwartz]

we didn't account for that we may initialize the Visual Studio-like compiler two times, once for a C compiler and once for the C++ compiler

This indicates that the original implementation was wrong because it mutated state shared between multiple compilers. The solution here isn't to catch and ignore the exception, but rather to fix this leaky behavior.

[fanc999]

Hi @eli-schwartz,

Thanks for the notes...

OK, so I guess I will try my best in doing this. So, maybe instead of removing the /utf-8 flag, I only add it to always_args when we are using CLang or Visual Studio 2015+, when it is not already in always_args. I guess, it makes better sense to me to only add a flag when we know that it can be supported from the beginning, but to drop the flag when we find out later that we need to use something that flag does not work with.

Anyways, not sure about the CI failings due to the brownouts...

[eli-schwartz]

Test failures seem relevant. In general, the underlying problem is the same whether you add them when wanted, or remove them when not wanted.

Namely, that one way or another a shared class attribute is getting modified in a manner that affects other instances, for example this could result in bad command lines where one instance does support /utf-8 and one does not.

[fanc999]

Hi @eli-schwartz,

Thanks for the lesson in Python on shared class attributes...:)! Let's see how CI looks at the code.

[eli-schwartz]

If this logic works, then setting it instead as self.always_args = self.always_args + ['/utf-8'] may be equivalent, in both cases __init__() will create a fresh list instead of mutating an existing one.

[fanc999]

Hi @eli-schwartz,

Indeed it is, interesting things in Python... :)