Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Many regular expression tests fail with CMake 3.31 #51

Closed
musicinmybrain opened this issue Dec 3, 2024 · 13 comments
Closed

Many regular expression tests fail with CMake 3.31 #51

musicinmybrain opened this issue Dec 3, 2024 · 13 comments
Labels

Comments

@musicinmybrain
Copy link

musicinmybrain commented Dec 3, 2024

https://bugzilla.redhat.com/show_bug.cgi?id=2330142

With CMake 3.30.5, as a “control” to show that I’m running the tests correctly:

$ gh repo clone editorconfig/editorconfig-core-py
$ cd editorconfig-core-py
$ python3.13 -m venv _e
$ . _e/bin/activate
(_e) $ pip install -e .
(_e) $ cmake .
(_e) $ ctest .
[…]
100% tests passed, 0 tests failed out of 197

Total Test time (real) =  56.53 sec

Ok, great! Now, upgrading CMake from 3.30.5 to 3.31.1, and re-running:

(_e) $ ctest .
[…]
90% tests passed, 20 tests failed out of 197

Total Test time (real) =  59.15 sec

The following tests FAILED:
          2 - meta_multiline (Failed)
          3 - star_single_ML (Failed)
          4 - star_zero_ML (Failed)
          5 - star_multiple_ML (Failed)
          7 - star_after_slash_ML (Failed)
          8 - star_matches_dot_file_after_slash_ML (Failed)
        131 - tab_width_default_ML (Failed)
        132 - tab_width_default_indent_size_tab_ML (Failed)
        133 - indent_size_default_ML (Failed)
        136 - indent_size_default_with_tab_width_ML (Failed)
        137 - lowercase_values1_ML (Failed)
        138 - lowercase_values2_ML (Failed)
        141 - repeat_sections_ML (Failed)
        142 - basic_cascade_ML (Failed)
        148 - blank_lines_between_properties_ML (Failed)
        152 - spaces_before_middle_property_ML (Failed)
        154 - comment_between_props_ML (Failed)
        158 - octothorpe_comment_between_props_ML (Failed)
        172 - parent_and_current_dir_ML (Failed)
        190 - unset_indent_size_ML (Failed)

All of the failing tests have output similar to

  2/197 Test   #2: meta_multiline ............................***Failed  Required regular expression not found. Regex=[^[
]*answer=42[
]+$
]  0.50 sec

I’m not sure quite what changed here, or how to fix it. I didn’t see anything obviously relevant in https://cmake.org/cmake/help/latest/release/3.31.html.

@musicinmybrain
Copy link
Author

Full output from ctest .

@cxw42
Copy link
Member

cxw42 commented Dec 10, 2024

If you clean, then re-do cmake . as well as ctest . with the new cmake version, does it work?

I would not personally expect files generated by one version of CMake to work exactly the same when processed by a different version of CMake.

@xuhdev
Copy link
Member

xuhdev commented Dec 11, 2024

Could you delete CMakeCache.txt, then run cmake . and post the output here?

@musicinmybrain
Copy link
Author

musicinmybrain commented Dec 11, 2024

I would not personally expect files generated by one version of CMake to work exactly the same when processed by a different version of CMake.

This is a good point. I should have made it clear in the original report that re-using the environment after upgrading CMake is not required to reproduce this, and I only did it to demonstrate that the regression is associated with CMake 3.31 rather than with something else in my environment. The results if I repeat the exercise in a completely clean checkout and use CMake 3.31.1 from the beginning are exactly as in the original report above. In fact, this originally appeared in Fedora’s python-editorconfig package, where RPM package builds happen in a clean chroot every time.

@cxw42
Copy link
Member

cxw42 commented Dec 12, 2024

@musicinmybrain thanks for the clarification and the bugzilla link! Just for my own understanding, how does "sphinx 8" come into the picture? Is that Sphinx the Python doc system?

@cxw42 cxw42 added the bug label Dec 12, 2024
@cxw42
Copy link
Member

cxw42 commented Dec 12, 2024

Well, this is the only regex-code diff I can find at the moment. I'm wondering if there's some strange truthiness rule we're triggering. (But that's just speculation on my part.)

@musicinmybrain
Copy link
Author

@musicinmybrain thanks for the clarification and the bugzilla link! Just for my own understanding, how does "sphinx 8" come into the picture? Is that Sphinx the Python doc system?

Right, the sequence of events was:

  1. The cmake package was updated to 3.31 in Fedora Rawhide. This caused python-editorconfig to start failing to build from source, but nobody noticed right away. (We have automated systems like Koschei to help with this, but failures to build from source still usually aren’t noticed immediately.)
  2. Tomáš Hrnčiar, working on a major-version upgrade of python-sphinx (Sphinx, the Python documentation system), conducted test rebuilds of everything that depended on it. Because the python-editorconfig package does use Sphinx to build documentation for a -doc subpackage, it was among those packages. Because python-editorconfig had just started failing to build from source, Tomáš filed an issue warning that it might be impacted by the Sphinx upgrade.
  3. I triaged the bug, read the actual error message, determined it was a CMake regression, altered the title in Bugzilla, and reported it upstream (here).

In the end, Sphinx is a red herring that has nothing to do with the problem. It just happened to be changing around the same time as CMake.

@cxw42
Copy link
Member

cxw42 commented Dec 12, 2024

Thanks! That makes sense. I did a few searches in cmake issues without success.

Does anybody have time to bisect this in cmake? I am happy to do so but it may be a while before I have time :( .

xuhdev added a commit to editorconfig/editorconfig-core-test that referenced this issue Dec 12, 2024
To keep them aligned with the main CMakeLists.txt file.

Seems to be the cause of editorconfig/editorconfig-core-py#51, since CMake warned about dropping compatibility below 3.10 a few versions ago.
@xuhdev
Copy link
Member

xuhdev commented Dec 12, 2024

#56 should have fixed this -- the CI passed with could you try with the latest master branch?

I'm not sure exactly how, but the fix (editorconfig/editorconfig-core-test#57) seems to resolve CMake's warning about dropping compatibility below 3.10 a few versions ago. This may still be a CMake bug because it should simply report an error if the min required version is below 3.10. Or, it can be a CMake regression that failed to implement an old policy somewhere between 3.5 and 3.16.

@xuhdev
Copy link
Member

xuhdev commented Dec 12, 2024

We can't remain v0.12.x after this fix: we will bump to v0.17, to keep up with the spec version, because there is a behavior change introduced in #56. If you need a fix for 0.12.x, please try to apply the patches in editorconfig/editorconfig-core-test#53 and editorconfig/editorconfig-core-test#57 to the tests/ directory.

@musicinmybrain
Copy link
Author

With CMake 3.31.1 and ef927af, I repeated:

$ git clean -fxd
$ python3.13 -m venv _e
$ . _e/bin/activate
(_e) $ pip install -e .
(_e) $ cmake .
(_e) $ ctest .
[…]
100% tests passed, 0 tests failed out of 202

Total Test time (real) =   4.20 sec

That looks good!

Then, as suggested, I backported editorconfig/editorconfig-core-test#53 and editorconfig/editorconfig-core-test#57 as patches for the Fedora package, and that worked too.

Thank you for investigating this! I’m surprised that bumping the minimum CMake version alone turned out to be a fix.

@xuhdev
Copy link
Member

xuhdev commented Dec 12, 2024

Perfect! Closing now

@xuhdev xuhdev closed this as completed Dec 12, 2024
@xuhdev
Copy link
Member

xuhdev commented Dec 12, 2024

Also published new version v0.17.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants