premake-core icon indicating copy to clipboard operation
premake-core copied to clipboard

Complex macro regression for gmake/codelite

Open Jarod42 opened this issue 3 years ago • 19 comments

What seems to be the problem?

Complex MACRO are broken at least with gmake/codelite in premake5 whereas they worked in premake4

defines {'COMPLEX_MACRO="void f() {}"'} (want to define function, not a string)

What did you expect to happen? That the macro is correctly transcript in generators.

so gcc -DCOMPLEX_MACRO="void f() {}"

What have you tried so far?

Workaround for gmake is to remove quote defines {'COMPLEX_MACRO=void f() {}'} but still produces error on codelite.

I might be ok with that syntax (but different behavior in premake4) as long as all generator accept it.

How can we reproduce this?

I have created https://github.com/Jarod42/premake-sample-projects on that purpose. it is project-01

What version of Premake are you using?

dev version of premake5 (taken by github action checkout) (and the one from apt-get install for premake4)

Jarod42 avatar Sep 17 '20 17:09 Jarod42

@Jarod42 is this in both gmake and gmake2, or just gmake?

nickclark2016 avatar Sep 26 '20 01:09 nickclark2016

Follow up @Jarod42: GCC wants it's function-like macros like: -D'COMPLEX_MACRO=void f() {}' rather than -DCOMPLEX_MACRO="void f() {}" (see https://gcc.gnu.org/onlinedocs/cpp/Invocation.html). So we have a few solution that could work here:

With no updates to the premake executable, you could do: defines { '\'COMPLEX_MACRO=void f() {}\'' }. I have written a test to ensure that this does work as expected, and it produces this: -D'MACRO(X)=fn(X)'.

function suite.defines_macro()
    defines "\'MACRO(X)=fn(X)\'"
    prepare()
    test.contains({ "-D\'MACRO(X)=fn(X)\'" }, gcc.getdefines(cfg.defines))
end

Now if we were to update Premake to support function-like macros, what I'd expect is to be able to define a macro like: defines { 'COMPLEX_MACRO=void f() {}' }, no fancy character escapes. By baking this directly into premake, however, users would need to remember that MSVC doesn't support function-like macros from the command line (see remarks section https://docs.microsoft.com/en-us/cpp/build/reference/d-preprocessor-definitions?view=vs-2019).

I'll defer to @samsinsane and @starkos to whether or not this is something that should be addressed. From my standpoint, this is working as intended from a gmake/gmake2 standpoint. I'm not familiar enough with codelite to confirm that this would be working as intended, but my guess says yes, since it defers to GCC by the looks of it.

nickclark2016 avatar Sep 27 '20 16:09 nickclark2016

I added gmake2 action too, same error. I didn't create vs github action yet.

single/double quotes are interpreted by shell, so -D'COMPLEX_MACRO=void f() {}' should be the same as -DCOMPLEX_MACRO="void f() {}", gcc example use parenthesis for functional macro. last way is indeed more generic.

I might be ok with any conventions that premake chooses (notice though that it changed from v4 to v5) as long as it doesn't change from generators. We also might want to have string or single char for MACRO. I think I should split my test project.

I just wanted to test projects with all generators for real.

About codelite, options are saved in xml form, so requires some escaping (different than shell's one).

Jarod42 avatar Sep 28 '20 14:09 Jarod42

I added gmake2 action too, same error. I didn't create vs github action yet.

single/double quotes are interpreted by shell, so -D'COMPLEX_MACRO=void f() {}' should be the same as -DCOMPLEX_MACRO="void f() {}", gcc example use parenthesis for functional macro. last way is indeed more generic.

I might be ok with any conventions that premake chooses (notice though that it changed from v4 to v5) as long as it doesn't change from generators. We also might want to have string or single char for MACRO. I think I should split my test project.

I just wanted to test projects with all generators for real.

About codelite, options are saved in xml form, so requires some escaping (different than shell's one).

Have you tried this? defines { '\'COMPLEX_MACRO=void f() {}\'' }

nickclark2016 avatar Oct 01 '20 16:10 nickclark2016

About Codelite, ignoring premake issues, it has issue with semicolon in MACRO and compiler options :( .

For gmake; I tried:

--     defines {"\'FOO=const char* foo() { return \"[]|\"; }\'"} -- Failed
--     defines {"'FOO=const char* foo() { return \"[]|\"; }'"} -- Failed
buildoptions {"-D'FOO=const char* foo() { return \"[]|\"; }'"} -- workaround

Jarod42 avatar Oct 08 '20 15:10 Jarod42

Does CodeLite support function like macros? If so, what should the generated project look like?

nickclark2016 avatar Oct 08 '20 16:10 nickclark2016

I have that which works (I replace ; by SEMICOLON as workaroung for bug inside codelite).

      <Compiler Options="-include &quot;../../src/force header.h&quot;;-D'BAR=const char* bar() { return &quot;[]|&quot; SEMICOLON }'" C_Options="" Assembler="" Required="yes" PreCompiledHeader="" PCHInCommandLine="no" PCHFlags="" PCHFlagsPolicy="0">
        <IncludePath Value="../../src/regular include"/>
        <Preprocessor Value="'FOO=const char* foo() { return &quot;[]|&quot; SEMICOLON }'"/>
      </Compiler>

Jarod42 avatar Oct 08 '20 16:10 Jarod42

Do quotes also need to be replaced by &quot; as well?

nickclark2016 avatar Oct 08 '20 17:10 nickclark2016

As it is in string (inside XML), yes. "defines" goes in "Preprocessor", "buildoptions" goes in "Compiler" "Options" for c++, C_Options for C.

Notice that I'm just user of Codelite, not developer of Codelite.

Jarod42 avatar Oct 08 '20 17:10 Jarod42

So it appears the macro is in both "Options" and "Preprocessor", is it fine to just fix this for preprocessor, or does it need to be fixed for both?

nickclark2016 avatar Oct 08 '20 17:10 nickclark2016

I tested both buildoptions and defines. Premake should handle both (up to you to prioritize bug resolution though ;) ) It is easier for me to use -D in buildoptions for testing purpose.

Jarod42 avatar Oct 08 '20 18:10 Jarod42

So I think the workaround you have for buildoptions is correct and working as intended (correct me if I'm wrong, but it works). I think defines is the correct way to approach this, so I'll be updating how defines are handled to replace all ; characters in a defines block with SEMICOLON. Is there a case that this breaks that you are aware of?

Referenced Workaround:

buildoptions {"-D'FOO=const char* foo() { return \"[]|\"; }'"} -- workaround

nickclark2016 avatar Oct 12 '20 16:10 nickclark2016

Codelite has just fixed their semicolon bug (I have to test it), so, Premake just has to fix escaping.

Jarod42 avatar Oct 13 '20 17:10 Jarod42

From the looks of it, premake already escapes quotation marks. There are tests that pass for defines with spaces in them. see. It doesn't appear that any escaping is done on the cxxflags (which is what buildoptions inputs into). see. We should probably be escaping all of these (cflags, cxxflags, etc), but I'm not 100% sure.

nickclark2016 avatar Oct 13 '20 20:10 nickclark2016

It is mostly my point about those unit tests where it is difficult (when possible) to see the issue in expected result:

<Preprocessor Value="ESCAPE=&quot;WITH\ SPACE&quot;"/>

Escaping space (\ ) inside (escaped) string is suspicious. Maybe it is the way to escape which is wrong as context free.

Meaning of quote also depends how it is interpreted by Codelite (when does it escape quote to pass to compiler, in project or at invocation? (Running Codelite is needed for that))

Is the resulting project working with it?

My primary concern is which syntax should be used in premake (as behavior has changed since premake4). Once it is decided, fixes can be done.

Jarod42 avatar Oct 14 '20 12:10 Jarod42

Yup. I'll defer to someone else to make the call, but the fix should be pretty straight forward once we know the path we want to take.

nickclark2016 avatar Oct 14 '20 12:10 nickclark2016

Escaping space (\ ) inside (escaped) string is suspicious.

From memory this was because of Codelite breaking when you had a space in a define, quoted or otherwise - this was a couple of years ago now, I would hope it no longer does this.

I don't understand what you're trying to say in the rest of your comment. If you want to fix something that's wrong with the Codelite generator, it's a very simple process:

  • Create a Codelite project using Codelite that works.
  • Write Premake tests to match snippets of this project.
  • Modify Premake code so tests pass.
  • Verify any changes required to existing tests due to these changes.

samsinsane avatar Oct 22 '20 13:10 samsinsane

@samsinsane: I know expected codelite/gmake result, but I have to know the wanted premake syntax (behavior has changed between premake4 and premake5, not sure it is intended)

As I said, currently we have:

For wanted: COMPLEX_MACRO ->void f() {} in premake4 (gmake/codelite): 'COMPLEX_MACRO="void f() {}"' (quotes) in premake5 (gmake): 'COMPLEX_MACRO=void f() {}' (no quotes)

What do you want as syntax? Think also about more complex cases, such as:

FOO -> const char* foo() { return "[]|"; }

  • defines('FOO=const char* foo() { return "[]|"; }') ?
  • defines('"FOO=const char* foo() { return \"[]|\"; }"') ?
  • defines('"FOO=const char* foo() { return \\\"[]|\\\"; }"') ?
  • ...

Jarod42 avatar Oct 30 '20 09:10 Jarod42

Related to #1216 and #1059

Jarod42 avatar May 17 '21 12:05 Jarod42