[libc++] Fixes lit portability issues.

[mordante]

@StephanTLavavej mentioned the libc++ tests no longer works for MSVC STL. The regex changes have been provided by Stephan.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

@StephanTLavavej mentioned the libc++ tests no longer works for MSVC STL. The regex changes have been provided by Stephan.

---

Full diff: https://github.com/llvm/llvm-project/pull/72435.diff

2 Files Affected:

• (modified) libcxx/test/lit.local.cfg (+7-1)
• (modified) libcxx/utils/libcxx/test/params.py (+2-2)

diff --git a/libcxx/test/lit.local.cfg b/libcxx/test/lit.local.cfg
index 4116553b6f7a9ae..da214edbccf5a88 100644
--- a/libcxx/test/lit.local.cfg
+++ b/libcxx/test/lit.local.cfg
@@ -29,7 +29,13 @@ def appendToSubstitution(substitutions, key, value):
     return [(k, v + " " + value) if k == key else (k, v) for (k, v) in substitutions]
 
 
-std = getSubstitution("%{cxx_std}", config)
+# External users of the LLVM test suite might not have set the substitutions.
+std = ""
+try:
+    std = getSubstitution("%{cxx_std}", config)
+except:
+    pass
+
 if std == "cxx26":
     std = "26"
 elif std == "cxx23":
diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index e31130572df6576..3fedbf972c0c822 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -114,7 +114,7 @@ def getStdFlag(cfg, std):
         ),
         actions=lambda std: [
             AddFeature(std),
-            AddSubstitution("%{cxx_std}", re.sub("\+", "x", std)),
+            AddSubstitution("%{cxx_std}", re.sub(r"\+", "x", std)),
             AddCompileFlag(lambda cfg: getStdFlag(cfg, std)),
         ],
     ),
@@ -187,7 +187,7 @@ def getStdFlag(cfg, std):
                 AddFeature("stdlib={}".format(stdlib)),
                 # Also add an umbrella feature 'stdlib=libc++' for all flavors of libc++, to simplify
                 # the test suite.
-                AddFeature("stdlib=libc++") if re.match(".+-libc\+\+", stdlib) else None,
+                AddFeature("stdlib=libc++") if re.match(r".+-libc\+\+", stdlib) else None,
             ],
         ),
     ),

[philnik777]

LGTM % nit.

[philnik777]

Suggested change

	std = ""
	try:
	std = getSubstitution("%{cxx_std}", config)
	except:
	pass
	try:
	std = getSubstitution("%{cxx_std}", config)
	except:
	std = ""

Seems like the more canonical way to me.

[ldionne]

This substitution definitely should be provided by whoever runs the test suite -- it's defined in params.py. Is there something I misunderstand here?

The rest of the changes look fine, but this one seems like guarding against something we don't support and should never happen.

[philnik777]

cxx_std is only ever used by libc++-specific tests, so I'm not sure it makes sense to require it to be provided.

[ldionne]

But still, my point is that folks need to be running the test suite via the normal entry points which includes params.py, and I don't think it makes sense to try to support anything else. Otherwise we're bound to break them on a regular basis and that's not good for anyone. @StephanTLavavej Are you folks going through our normal entry point for Lit or do you still maintain your own custom Lit test format?

[StephanTLavavej]

I don't have the best understanding of how we interface with Lit (i.e. how to recognize the "normal entry point"), but it's the stuff in https://github.com/microsoft/STL/tree/46843b3bf320c6070ff63e978936bcf898e5a2bc/tests/utils . We invoke our stl-lit.py generated from https://github.com/microsoft/STL/blob/46843b3bf320c6070ff63e978936bcf898e5a2bc/tests/utils/stl-lit/stl-lit.in and that eventually uses our params.py at https://github.com/microsoft/STL/blob/46843b3bf320c6070ff63e978936bcf898e5a2bc/tests/utils/stl/test/params.py . If there's something we should add there or to another file, I would be happy to do so.

(I think the raw string regex changes in this PR are independently needed, though.)

[ldionne]

Oh, hmmm yeah that's quite a setup you folks have there. Normally, the intended way to use the test suite is to provide only this config file: https://github.com/llvm/llvm-project/blob/main/libcxx/test/configs/ibm-libc%2B%2B-shared.cfg.in (for example).

You set up the flags that are required for your stdlib, compiler, platform, etc, and then everything else should follow from that. I'm not sure why you're doing something significantly different here but I'd like to understand (and perhaps we can change a few things so that you folks can drop your custom format). As it stands, IIUC you basically use the contents of our test suite but almost none of our "Lit setup", and I think supporting that reliably is pretty much a lost battle.

The raw string regexes are definitely a non-controversial improvement. This change here I'm not like "over my dead body" against, but I think it does beg the question of how the test suite is actually being consumed and we should figure out the answer to that (and agree on what we want to support).

[StephanTLavavej]

We have files like https://github.com/microsoft/STL/blob/46843b3bf320c6070ff63e978936bcf898e5a2bc/tests/std/lit.site.cfg.in . It sounds like we might need to add config.substitutions.append(("%{cxx_std}", "cxx26")) or maybe lit_config.BLAH?

I'm not sure why you're doing something significantly different here

Me neither - @CaseyCarter might know more though.

[StephanTLavavej]

Ok, I think I can fix this on the microsoft/STL side! If I add

config.substitutions.append(('%{cxx_std}', ''))

to our tests\std\lit.site.cfg.in, tests\tr1\lit.site.cfg.in, tests\libcxx\lit.site.cfg.in, that unblocks the test pass.

[mordante]

@philnik777 Your suggestion changes the scope of std, which means the code no longer works. (In general I agree that your suggestion is the typical way to write it.)

@ldionne I was not sure we required all substitutions. AFAIK they are not documented and changes to them not mentioned in the release notes. I'll undo the changes to this file.

[ldionne]

This substitution definitely should be provided by whoever runs the test suite -- it's defined in params.py. Is there something I misunderstand here?

The rest of the changes look fine, but this one seems like guarding against something we don't support and should never happen.

[StephanTLavavej]

Looks like the CI failure here was recently fixed by #72480.

[ldionne]

This LGTM, this part of the changes is definitely not controversial. @StephanTLavavej LMK if you'd like to work on making it possible for STL to use our official entry points for testing, I'd be happy to provide assistance.

[StephanTLavavej]

Thanks! Yeah, I am definitely interested in using your official entry points, I’ll just need to find some time when I’m not completely overloaded with other work - the WG21 treadmill is neverending 😹