Allow certain command line settings to be additive

[sbc100]

For certain command line settings (specifically most settings that take a list) it is useful to be able to add to the existing setting rather than replace it.

For example, the -sEXPORTED_FUNCTIONS argument. In a build system where different components can inject link flags it makes sense that each component be able add to the list of exported functions without clobbering the current list.

Fixes: #19058

[kripken]

This seems a little risky to me, as we have some settings that we want people to be able to replace/shrink, like INCOMING_MODULE_JS_API. How about this notation instead?

-sEXPORTED_FUNCTIONS+=foo

That is, += appends rather than assigns/replaces.

[sbc100]

I don't think anyone ever wants one command line argument to replace/shrink the same argument already specified on the same line.

In the case of INCOMING_MODULE_JS_API we want the command line arg to replace the default set, but if one library wants to set -sINCOMING_MODULE_JS_API=foo and another sets -sINCOMING_MODULE_JS_API=bar I'm pretty sure we always want them to be additive with the result being foo,bar.

The way I have set it up is that this happens on a per-setting basis so it doesn't have to apply to all of them.

[kripken]

I see what you mean. INCOMING_MODULE_JS_API wouldn't be used from a library anyhow, I guess, that's a global setting for an application, so you wouldn't apply this to that setting. While it does make sense for exported functions, so you'd apply it there. Which I see is what this PR does.

Still, what do you think about using += notation? I think that is less ambiguous. As a side benefit, it would avoid any backwards compatibility risk whatsoever.

[sbc100]

I just can think of a situation where you wouldn't want += to be the default, and it add more complexity to the command line parser. If we ever find an option where we don't want it to be the default perhaps we could add it then?

[kripken]

Well, what worries me is that this is a breaking change. I do see your point that this is the more natural behavior, but it can break user's builds if they had the same flag twice. Build systems are weird and sometimes that duplication happens (in Binaryen for whatever reason there is more than one -Ox flag, for example...).

Less concerning but still a little odd is that some settings would have this behavior and others wouldn't. And we already have some settings that are additive in their name, like EXTRA_*. So this feels like it could be more consistent with those somehow, but I'm not sure how. Perhaps EXTRA_EXPORTED_FUNCTIONS would be additive, for example?

[vkirilyuk]

It's a source of bugs when using bazel: cc_library sets some flags for it's functionality (say, requests ccall). The other library / cc_binary wants to access filesystem, so adds FS. And for some reason it doesn't work and there is no warning why it's happening. To find the problem I literary had to understand how emcc flags and build system works, and even then there is no solution. (Actually, there is one: abuse EXTRA_EXPORTED_FUNCTIONS because while the flag is deprecated, it's passed through and appended in the end. But my colleague uses it too in his library, so no way for me to set custom flags at all).

But I agree that it's a breaking change, so my proposal is to add a warning that overriding is happening and perhaps offer a flag to turn additive functionality on. Something like this:

  for s in settings_changes:
    key, value = s.split('=', 1)
    key, value = normalize_boolean_setting(key, value)
    if key in user_settings and user_settings[key] != value:
      diagnostics.warning('unused-command-line-argument', f'{key} was provided multiple times with different values. Previous value: {user_settings[key]}. New value: {value}.')
    user_settings[key] = value

[sbc100]

Adding in #21464

[trybka]

I see what you mean. INCOMING_MODULE_JS_API wouldn't be used from a library anyhow, I guess, that's a global setting for an application, so you wouldn't apply this to that setting. While it does make sense for exported functions, so you'd apply it there. Which I see is what this PR does.

Why not? It seems quite natural to have a library (say using inline JS) declare what module.js API functions it uses... and then to propagate that up. In a Bazel world, libraries are responsible for declaring their dependencies, and a used API feels like a dependency.

Still, what do you think about using += notation? I think that is less ambiguous. As a side benefit, it would avoid any backwards compatibility risk whatsoever.

I don't know of any prior art of command-line flags behaving like this. It definitely does not feel ergonomic.
While I agree some folks may be relying on this behavior, I'd argue that the old behavior was unnatural. Yes, some flags (like -O) only expect a single value, and last value wins. But anything that takes a list really ought to accumulate. (It would be weird if, e.g. -l didn't accumulate for instance).

Maybe allow a synthetic, implicit prefix for each list flag, like ADD_ e.g. ADD_EXPORTED_FUNCTIONS.
If it's a list, it also supports this thing. Then at least build systems that want to leverage this can.

[sbc100]

Why not? It seems quite natural to have a library (say using inline JS) declare what module.js API functions it uses... and then to propagate that up. In a Bazel world, libraries are responsible for declaring their dependencies, and a used API feels like a dependency.

Normally its not user code that reads things from INCOMING_MODULE_JS_API but emscripten's own code JS code. Its not really something that was designed to be extensible.

The usage for INCOMING_MODULE_JS_API is really about what the HTML surrounding the program is going to provide. Normally a library doesn't know that, only the person building the final binary. The person in charge of linking the program would ask themselves "am I going to ever provide such as paramater to the program".. if the answer is no, then then there is no need to add that to the INCOMING_MODULE_JS_API list. Adding something to that list, but never supplying it at runtime is just a waste of code.

I can imagine there might be exceptions.. but that seems like the rule to me.