Skip to content

GUL14 migration: Preserve gul14/catch.h includes#34

Merged
alt-graph merged 3 commits intomainfrom
preserve_gul14_catch_includes
Jun 16, 2025
Merged

GUL14 migration: Preserve gul14/catch.h includes#34
alt-graph merged 3 commits intomainfrom
preserve_gul14_catch_includes

Conversation

@alt-graph
Copy link
Copy Markdown
Member

GUL17 does not bundle a header-only Catch2 include file anymore, so some manual work is required to migrate a project to Catch2v3. This is out of the scope of the GUL14 migration script. The best way of dealing with this is, arguably, to keep including the header that is bundled with GUL14.

[why]
GUL17 does not bundle a header-only Catch2 include file anymore, so some
manual work is required to migrate a project to Catch2v3. This is out of
the scope of the GUL14 migration script. The best way of dealing with
this is, arguably, to keep including the header that is bundled with
GUL14.

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
@alt-graph alt-graph requested a review from Copilot June 6, 2025 07:02
@alt-graph alt-graph self-assigned this Jun 6, 2025
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a special-case sed rule in the GUL14→GUL17 migration script to preserve the legacy Catch2 header.

  • Inserts a pattern to rewrite #include <gul17/catch.h> back to gul14/catch.h
  • Keeps existing namespace and header migrations intact
Comments suppressed due to low confidence (3)

tools/migrate_gul14_to_gul17.bash:53

  • [nitpick] Add a comment above this line explaining that GUL17 no longer bundles the Catch2 header, so we intentionally reroute includes back to gul14/catch.h.
-        -e 's/(#include\s*["<])gul17\/catch.h([">])/\1gul14\/catch.h\2/g' \

tools/migrate_gul14_to_gul17.bash:53

  • [nitpick] Anchor the regex to the start of the line (e.g., ^) so only actual include directives are matched and you avoid unintended replacements in other contexts.
-        -e 's/(#include\s*["<])gul17\/catch.h([">])/\1gul14\/catch.h\2/g' \

tools/migrate_gul14_to_gul17.bash:53

  • [nitpick] Consider adding or updating a test case in the migration script's suite to verify that #include <gul17/catch.h> is correctly rewritten to gul14/catch.h.
-        -e 's/(#include\s*["<])gul17\/catch.h([">])/\1gul14\/catch.h\2/g' \

[why]
To reflect that the script should be called on the main directory of a
project.

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
@alt-graph
Copy link
Copy Markdown
Member Author

Since this is still open, I have added two more commits to

  • improve the help text, and
  • auto-convert the Gitlab CI files as well.

@Finii
Copy link
Copy Markdown
Member

Finii commented Jun 16, 2025

If we keep the gul14/catch.h usage, then the dependency-rewrite in the gitlab ci seems to be problematic?

handle_gitlab_ci_file() {
    sed -i -E -e 's/dev-doocs-libgul14/gul17-dev/g' "$1"
}

Maybe it should then just add the gul17-dev instead of replacing 🤔

This new substitution line

 -e 's/(#include\s*["<])gul17\/catch.h([">])/\1gul14\/catch.h\2/g' \

is it to revert previous autoconversions?

@Finii
Copy link
Copy Markdown
Member

Finii commented Jun 16, 2025

Hmm, neither client nor serverlib install GUL in the Gitlab-ci (they fall back to selfbuild HEAD), and servers usually install the latest serverlib package/release, is there even someone who installs GUL manually?

@alt-graph
Copy link
Copy Markdown
Member Author

Hmm, neither client nor serverlib install GUL in the Gitlab-ci (they fall back to selfbuild HEAD), and servers usually install the latest serverlib package/release, is there even someone who installs GUL manually?

Olaf asked if we could do that for the CI files. Apparently it is used in several libraries.

@alt-graph
Copy link
Copy Markdown
Member Author

This new substitution line [...] is it to revert previous autoconversions?

In the same sed command, we first replace include <gul14...> by include <gul17...> for everything. Then we replace the catch2 headers back. It seemed just the easiest way to do that.

@alt-graph
Copy link
Copy Markdown
Member Author

If we keep the gul14/catch.h usage, then the dependency-rewrite in the gitlab ci seems to be problematic?

Yes, maybe. But honestly, I do not care too much. People should migrate to Catch2v3 anyhow, maybe we do not have to cover each and every corner case with this script. 🤷

@alt-graph
Copy link
Copy Markdown
Member Author

Thanks, @Finii!

@alt-graph alt-graph merged commit 510d831 into main Jun 16, 2025
3 checks passed
@alt-graph alt-graph deleted the preserve_gul14_catch_includes branch June 16, 2025 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants