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

do @ variable substitution when parsing cmake format #13987

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Jan200101
Copy link
Contributor

the upstream behavior of configure_file in cmake parses both @var@ and ${VAR} and only supports disabling the bracket variables through the @ONLY argument, meson needs to follow this behavior for compatibility

this is an ugly solution but I think its best considering the alternatives:

  • calling do_conf_str twice with both formats
    • will break if the first iteration places substitutes that match the format of the second iteration
  • adapt the existing bracket format regex to support @
    • the existing regex is unholy and broken in more ways than I am willing to list, it would be better to completely gut this part of the code and replace it with regular python code
  • rewrite the code to not use regex
    • this would be quite complex and I am not familiar enough with mesondefine to be able to find any regressions. The lack of specification on how its suppose to work does not help either

Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this, I had noticed this but never had time to fix it.

It would be really good to get test coverage for this in the test cases/common/14 configure file test

mesonbuild/utils/universal.py Outdated Show resolved Hide resolved
the upstream behavior of configure_file in cmake parses both @var@ and ${VAR} and only supports disabling the bracket variables through the `@ONLY` argument, meson needs to follow this behavior for compatibility
@Jan200101
Copy link
Contributor Author

Jan200101 commented Dec 10, 2024

rebasing to un-screw CI
EDIT: it ended up becoming worse, somehow, I don't think this is caused by this PR

@eli-schwartz
Copy link
Member

rebasing to un-screw CI
EDIT: it ended up becoming worse, somehow, I don't think this is caused by this PR

mkenums/main6.c:8:42: error: ‘MESON_TYPE_THE_XENUM’ undeclared (first use in this function)

This is not a known or preexisting failure. I would venture to say something about this PR is in conflict with behavior that that test case was relying on, but I haven't had a chance to review the implementation.

@eli-schwartz
Copy link
Member

We have a file, enums.h.in:

G_BEGIN_DECLS
/*** END file-header ***/
/*** BEGIN file-production ***/
/* enumerations from "@basename@" */
/*** END file-production ***/
/*** BEGIN value-header ***/
GType @enum_name@_get_type(void) G_GNUC_CONST;
#define @ENUMPREFIX@_TYPE_@ENUMSHORT@ (@enum_name@_get_type())
/*** END value-header ***/
/*** BEGIN file-tail ***/
G_END_DECLS

We convert it to enums6.h.in and then use it as a template for glib-mkenums:

# Simple trick to copy the file without substitutions, can be
# removed when https://github.com/mesonbuild/meson/pull/3383 is fixed
gen_h_template = configure_file(input: 'enums.h.in',
output: 'enums6.h.in',
configuration: configuration_data(),
format: 'cmake')
enums_h6 = gnome.mkenums('enums6',
sources : 'meson-sample.h',
h_template : gen_h_template,
ftail : '/* trailing header file info */',
install_header : true,
install_dir : get_option('includedir'))

The replacement tokens must be parsed by glib-mkenums, not configure_file. We use configure_file in order to copy a file without modifications, and that's what this PR causes to stop behaving the way it used to behave. If you configure the test case before and after this PR:

File created by glib-mkenums:

diff --git a/builddir-old/mkenums/enums6.h b/builddir-new/mkenums/enums6.h
index 313d846ff..950175623 100644
--- a/builddir-old/mkenums/enums6.h
+++ b/builddir-new/mkenums/enums6.h
@@ -8,11 +8,11 @@
 
 G_BEGIN_DECLS
 
-/* enumerations from "meson-sample.h" */
-GType meson_the_xenum_get_type(void) G_GNUC_CONST;
-#define MESON_TYPE_THE_XENUM (meson_the_xenum_get_type())
-GType meson_the_flags_enum_get_type(void) G_GNUC_CONST;
-#define MESON_TYPE_THE_FLAGS_ENUM (meson_the_flags_enum_get_type())
+/* enumerations from "" */
+GType _get_type(void) G_GNUC_CONST;
+#define _TYPE_ (_get_type())
+GType _get_type(void) G_GNUC_CONST;
+#define _TYPE_ (_get_type())
 
 G_END_DECLS
 

File created by configure_file:

diff --git a/builddir-old/mkenums/enums6.h.in b/builddir-new/mkenums/enums6.h.in
index 479867fd1..7f6a50107 100644
--- a/builddir-old/mkenums/enums6.h.in
+++ b/builddir-new/mkenums/enums6.h.in
@@ -9,11 +9,11 @@ G_BEGIN_DECLS
 
 /*** BEGIN file-production ***/
 
-/* enumerations from "@basename@" */
+/* enumerations from "" */
 /*** END file-production ***/
 /*** BEGIN value-header ***/
-GType @enum_name@_get_type(void) G_GNUC_CONST;
-#define @ENUMPREFIX@_TYPE_@ENUMSHORT@ (@enum_name@_get_type())
+GType _get_type(void) G_GNUC_CONST;
+#define _TYPE_ (_get_type())
 /*** END value-header ***/
 
 /*** BEGIN file-tail ***/

@eli-schwartz
Copy link
Member

I think what this means is that the existing testcase code was relying on format: 'cmake' not actually implementing cmake replacements, and obviously implementing that missing feature meant that we then mangled the file by, well, copying it.

Or something.

Those @VARNAME@ tokens weren't part of the empty configuration_data used, obviously, so they ended up being deleted.

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.

configure_file with format cmake doesn't handle @var@ like CMake does
3 participants