[CMake] Add sanity tests for python file lists#15670
Conversation
|
Can one of the admins verify this patch? |
| test.entries = [] | ||
| test.whitelist = [] | ||
|
|
||
| for line in lines: |
There was a problem hiding this comment.
I find this logic confusing and (maybe because I don't understand it) it looks brittle -- are the comments becoming meaningful?
|
Why should we add the new syntax? Why not make the test slightly more flexible, and just comment out the lines we use the special syntax instead? My concern is the syntax is specific to this, and will be alien to anyone adding new modules. So they will just "hack" the test instead of trying to write the special syntax. |
|
|
This can be as simple as "If some folder is left out from this list, we do not care about them" and "if something is commented, we know these should exist in our python packages, but there are bugs with these packages, and they will be added to the pip package as soon as possible. |
|
The CMake code looks fine to me, but I'll defer to @martinwicke or @gunan about what we'd like to support in the build. |
martinwicke
left a comment
There was a problem hiding this comment.
See, e.g., line 292 of python_modules.txt. Will a plain comment work with the new logic? I suppose it's added to the whitelist, which is harmless?
I'm ok with this, although it's a tiny bit ugly. I would like this behavior to be documented somewhere though. The python_modules.txt would be ideal (if we can make that work, given comments are meaningful now), and the test should, if it fails, explains how one can fix it.
|
|
Great. The only question left is runtime, but I don't expect problems for this. |
Replaces #15166
See discussion in #15368
@gunan @martinwicke Friendly ping.