Add support for generating a compile_commands.json for c-dependencies/js-compute-runtime

[elliottt]

Add a new compile_commands.json target in c-dependencies/js-compute-runtime/Makefile for generating the build config that clangd requires. Typing make compile_commands.json will generate the file, and enable the use of clangd when working on js-compute-runtime.

Also make some small changes to the Makefile:

• Replace uses of = with := when possible to avoid accidentally introducing macros
• Add clean and distclean targets
• Add entries for generated intermediates to .gitignore

[jameysharp]

Lots of good improvements here! I'm not immediately convinced that the changes to WASM_STRIP are correct, but all my other notes are quibbles you can feel free to ignore.

[jameysharp]

I'm curious how you feel about placing these gitignore entries in c-dependencies/js-compute-runtime/.gitignore instead. It's mostly a question of style/personal preference and I don't feel strongly about it. But I personally tend to like keeping ignore rules closer to the part of the source tree that generates the files to be ignored. For me the main exception is when there are multiple subtrees that need to ignore the same things; I might put things like *.o at top-level if there are multiple separate C projects in-tree.

[elliottt]

I like that too, but have seen preferences elsewhere for one global .gitignore. Why don't we move these into c-dependencies/js-compute-runtime/.gitignore and we can migrate rules like *.o out if we start accumulating more c++ source outside of that tree?

[jameysharp]

If MODE and CARGO_FLAG are assigned with := above, should they be assigned that way here too?

[elliottt]

Good catch, they definitely should be :)

[jameysharp]

I haven't touched makefiles in a while, but the GNU make manual says execution within a rule stops at the first line that exits with a non-zero status. So combining two lines with && is unnecessary and I think makes maintenance a little harder.

[elliottt]

This avoids the overhead of invoking the shell once per line, but that's probably not really something worth worrying about here. I'll switch it back because I agree that it's more readable on a separate line 👍

There is the .ONESHELL directive, but that brings back the opportunity for a command to fail and not halt rule execution.

[jameysharp]

Does $1 work here? I see you've changed the usage of this variable to a $(call), but given that my reading of the GNU make manual (call function, variable references) is that you'd need to use $(1) rather than $1 here.

I'm not sure either change is necessary anyway: $@ means the target of the current rule, which should be the right thing here, shouldn't it?

[elliottt]

For variables with only a single character name, it's fine to use them without parens. This makefile behaves the same for calls to both ECHO and ECHO2:

ECHO = echo $1                                                                       
ECHO2 = echo $(1)                                                                    
                                                                                     
foo: ci/*.sh                                                                         
        $(call ECHO,$<)                                                              
        $(call ECHO2,$<) 

I was a little bit uneasy leaving it as $@ because it makes assumptions about where the WASM_STRIP macro will be used (always in a rule context), while being explicit about it being a macro and using $1 felt like it encoded the intent a little better.

I sort of wish that make didn't allow macros to be introduced with = :(

[jameysharp]

Ah, I missed the last paragraph of the variable references section. Got it.

I wonder, could the conditional be shoved into the rule directly in a way that's more clear? I dunno, maybe like this:

	test -z "$(findstring g,$(CXX_OPT))" && which wasm-opt >/dev/null && wasm-opt --strip-debug -o $@ $@

I'm not convinced that looking for the single letter "g" makes sense here either, but maybe it's best to leave that alone. I suppose it works if you pass either -Og or -g?

[elliottt]

It'll definitely match unrelated occurrences of g in CXX_OPT. We could search for explicitly -g, but that will still match substrings.

[jameysharp]

I think you could safely add $(FSM_CPP) to the dependencies of this rule to express what this comment says.

[elliottt]

I didn't want it to depend on the files, only the state of that variable. I could pipe them into the compiler_flags file as well though...

[jameysharp]

Oh right, I get it. Well, maybe the rule runs fast enough that you could mark it as .PHONY and not worry about its dependencies?

[elliottt]

Entirely reasonable! I'll make that change.

[jameysharp]

All I know about compile_commands.json is that when I tried to generate one once I got confused and gave up. Is this the best way to do it? I thought I remembered clang having an option to generate it for you, maybe?

[elliottt]

If clang can do it automatically, that would be better. This version doesn't feel ideal, but does produce a working compile_commands.json :)

[elliottt]

A quick googling didn't turn up anything that automatically generates compile_commands.json, and my experience with compile_commands.json from bazel and cmake is that it's generated by the build system, not clang/clangd. How do you feel about using this hacky version for now, and replacing it with something more robust when we find it?

[jameysharp]

If we find it. It's entirely possible I gave up the last time because there isn't a nice option if your build system doesn't do it for you; I don't remember. Yeah, I'm satisfied, ship it!

Maybe we should discuss changing the build system here to something more capable. That's certainly not blocking this PR though.