Fix release name macro expansion

[netmindz]

Fix for #4260

[softhack007]

Error: The path for one of the files in artifact is not valid: /WLED_0.15.0-b7_"ESP8266".bin. Contains the following character: Double quote "

@netmindz I think output_bins.py needs to be adjusted - might be that simply deleting any " and ' from the release_name before copy files might be sufficient.

[blazoncek]

I'm wondering what's wrong with stringification? https://gcc.gnu.org/onlinedocs/gcc-4.8.5/cpp/Stringification.html

There are some other places that use TOSTRING() too.

[willmmiles]

I'm wondering what's wrong with stringification? https://gcc.gnu.org/onlinedocs/gcc-4.8.5/cpp/Stringification.html

There are some other places that use TOSTRING() too.

It macro-expands recursively -- there is no way to expand exactly once. Eg: -DESP32=Stuff -D WLED_RELEASE_NAME=ESP32 means that TOSTRING(WLED_RELEASE_NAME) expands to "Stuff" -- eg WLED_RELEASE_NAME->ESP32->Stuff. Since ESP32 happens to be #defined somewhere (FastLED, if nowhere else!), it expands to something unhelpful before it's converted to a string literal.

[blazoncek]

It macro-expands recursively -- there is no way to expand exactly once. Eg: -DESP32=Stuff -D WLED_RELEASE_NAME=ESP32 means that TOSTRING(WLED_RELEASE_NAME) expands to "Stuff" -- eg WLED_RELEASE_NAME->ESP32->Stuff. Since ESP32 happens to be #defined somewhere (FastLED, if nowhere else!), it expands to something unhelpful before it's converted to a string literal.

That explains 1 seen in builds.

So the simplest solution is to use something unique that has no ordinary/recursive counterpart. I.e. rename "ESP32" with "ESP32plain" in platformio.ini. IMO that's acceptable as no previous builds have "ESP32" (or "ESP8266") compiled in due to the recursion flaw.

This (adding unique WLED_RELEASE_NAME) would also help differentiate builds @Moustachauve wants to implement in his mobile app.

[netmindz]

It macro-expands recursively -- there is no way to expand exactly once. Eg: -DESP32=Stuff -D WLED_RELEASE_NAME=ESP32 means that TOSTRING(WLED_RELEASE_NAME) expands to "Stuff" -- eg WLED_RELEASE_NAME->ESP32->Stuff. Since ESP32 happens to be #defined somewhere (FastLED, if nowhere else!), it expands to something unhelpful before it's converted to a string literal.

That explains 1 seen in builds.

So the simplest solution is to use something unique that has no ordinary/recursive counterpart. I.e. rename "ESP32" with "ESP32plain" in platformio.ini. IMO that's acceptable as no previous builds have "ESP32" (or "ESP8266") compiled in due to the recursion flaw.

This (adding unique WLED_RELEASE_NAME) would also help differentiate builds @Moustachauve wants to implement in his mobile app.

My initial version of this PR changed the value to be WLED_ESP32, the removed the hard coded WLED from the rename python script, but that's a bigger change, hence swapping to @willmmiles suggestion for a smaller change

[blazoncek]

Or perhaps just:

#ifndef WLED_RELEASE_NAME
  #define WLED_RELEASE_NAME dev_release
#else
  #if WLED_RELEASE_NAME == ESP32 || WLED_RELEASE_NAME == ESP8266
    #error Wrong WLED_RELEASE_NAME.
  #endif
#endif

[netmindz]

Or perhaps just:

#ifndef WLED_RELEASE_NAME
  #define WLED_RELEASE_NAME dev_release
#else
  #if WLED_RELEASE_NAME == ESP32 || WLED_RELEASE_NAME == ESP8266
    #error Wrong WLED_RELEASE_NAME.
  #endif
#endif

That is still fragile, you would have to add a new value to the if for every new release name

[blazoncek]

That is still fragile, you would have to add a new value to the if for every new release name

Only as fragile as use of pre-existing macros is. If platformio.ini is updated with unique release names (denoting build options for example, I'm sure you are familiar with MM type of build options and releases) no change anywhere in the code is necessary (meaning no sophisticated testing necessary).

For example WLED_RELEASE_NAME could be:

• ESP8266_plain
• ESP8266_common_usermods
• ESP8266_audio
• ESP8266_compat
• ESP8266_160MHz
• ESP01_no_OTA
• ESP02_OTA
• ESP32_plain
• ESP32_common_usermods
• ESP32_audio
• ESP32_8M_plain
• etc.

[blazoncek]

Thinking more of it, it really does not matter how users construct their WLED_RELEASE_NAME (using ESP32 or not). What matters (for the intention @Moustachauve has) is the uniqueness of official releases (so he can distinguish when determining if update is available). If the string (whatever its content is) does not match no official update is available. And having WLED_RELEASE_NAME=1 is still a valid value if binary with such name exists.

[Moustachauve]

The most recent version will try to use that value to match a binary file, so in the best world it should fit one of the binary in the GitHub release .

It could indeed be any value that someone wanted (let's say they made a hypothetical "christmas" version and a "Halloween" version, whatever)

[willmmiles]

If I may steelman the counter-position: A major down side of this change is that quotes are now required for WLED_RELEASE_NAME. This is a breaking change for many of us who ended up copy-and-pasting old build_flags to our platform_override.ini to build custom targets. (This includes me!) One might call it an abuse of WLED_RELEASE_NAME, but I certainly didn't know better -- I'd just copied or extended the flags from existing environments; I didn't want to spend a lot of time trying to figure out which ones were important and which were informational. I'd just wanted to hack up some code for my projects; and I doubt I'm the only one. I can appreciate that making this change will require attention from just about everyone making a new custom build.

That said: I do think requiring quotes is the best possible option; macro expansion in this value is ultimately going to just continue to cause confusion in the future. If we're going to make breaking changes, now (before the 0.15.0 release) is as good a time as any.

[softhack007]

That said: I do think requiring quotes is the best possible option; macro expansion in this value is ultimately going to just continue to cause confusion in the future.

I agree, too. Creating a list of "bad names" that lead to error is a neverending task. The next user may call his build MAX or NETWORK or M_PI, WLED_FS or whatever ... we can't catch all these macros in advance. So better to introduce quotes now and have a solution that's robust.

[netmindz]

Just for completeness, this would be the the alternative option that doesn't require the quotes and uses values that are unlikely to ever expand

#4317

[softhack007]

Looks good, did a few build tests and works as expected 👍
I found two minor improvements you could make:

a) missed one place in platformio_override.sample.ini:

WLED/platformio_override.sample.ini

Lines 37 to 43 in 5b989ad

	; *** To use the below defines/overrides, copy and paste each onto it's own line just below build_flags in the section above.
	;
	; Set a release name that may be used to distinguish required binary for flashing
	; -D WLED_RELEASE_NAME=ESP32_MULTI_USREMODS
	;
	; disable specific features
	; -D WLED_DISABLE_OTA

b) you could add a helping comment to this line - actually compilation will error here when WLED_RELEASE_NAME is missing quotes

WLED/wled00/wled.h

Line 272 in 5b989ad

WLED_GLOBAL char releaseString[] _INIT(TOSTRING(WLED_RELEASE_NAME)); // somehow this will not work if using "const char releaseString[]

typical compiler errors:

In file included from wled00/wled.cpp:2:0:
<command-line>:0:19: error: 'ESP32_livingroom' was not declared in this scope
wled00/wled.h:253:22: note: in definition of macro '_INIT'
   #define _INIT(x) = x
                      ^
wled00/wled.h:272:40: note: in expansion of macro 'WLED_RELEASE_NAME'
 WLED_GLOBAL char releaseString[] _INIT(WLED_RELEASE_NAME); // somehow this will not work if using "const char releaseString[]

or

In file included from wled00/wled.cpp:2:0:
<command-line>:0:7: error: initializer fails to determine size of 'releaseString'
wled00/wled.h:253:22: note: in definition of macro '_INIT'
   #define _INIT(x) = x
                      ^
<command-line>:0:19: note: in expansion of macro 'ESP32'
wled00/wled.h:272:40: note: in expansion of macro 'WLED_RELEASE_NAME'
 WLED_GLOBAL char releaseString[] _INIT(WLED_RELEASE_NAME); // somehow this will not work if using "const char releaseString[]
                                        ^
<command-line>:0:7: error: array must be initialized with a brace-enclosed initializer
wled00/wled.h:253:22: note: in definition of macro '_INIT'
   #define _INIT(x) = x
                      ^
<command-line>:0:19: note: in expansion of macro 'ESP32'
wled00/wled.h:272:40: note: in expansion of macro 'WLED_RELEASE_NAME'