Add RGBASM -MC flag to continue -MG after missing dependency files

[Rangi42]

Fixes #903

RGBASM -M outputs a list of make dependencies (i.e. INCLUDEd or INCBINned files) instead of an object file. The -MG flag allows outputting dependencies for nonexistent files, on the user's implied assumption that any nonexistent files will be generated by make. However, RGBASM exits after outputting the first such nonexistent dependency, since its presence could potentially influence how subsequent code is assembled. The user thus needs to run rgbasm -M ... -MG in a loop until all dependencies are covered, which is a "finicky" process that can require advanced knowledge of your build system (presumably make).

This PR adds a -MC flag to allow continuing processing even after a nonexistent dependency, on the user's implied assumption that they're not doing anything tricky/weird that the dependency would have influenced. The goal is to make a "typical" workflow -- for example, a .asm file that just INCBINs a lot of graphics files, but has no weird tricky conditionals about their byte sizes -- easier to handle. And it's still opt-in since in those tricky cases, this behavior would be unreliable.

[Rangi42]

Some examples of where -MC would not be appropriate:

DEF MORE = 0
; code.inc should be generated before assembling, and might do `DEF MORE = 1`
INCLUDE "code.inc"
; if code.inc did `DEF MORE = 1`, more-code.inc should be generated too
if MORE == 1
	INCLUDE "more-code.inc"
endc

Data:
; data.bin should be generated before assembling, but might be empty
INCBIN "data.bin"
; if data.bin was empty, alternate-data.bin should be generated too
if @ - Data == 0
	INCBIN "alternate-data.bin"
endc

In those cases, rgbasm -MG -MC would cause different dependencies to be output than if rgbasm -MG had exited after the first nonexistent one, let make generate that first one, and then re-ran rgbasm -MG.

[Rangi42]

In #903 @daid said "I currently first run rgbasm and then append the resulting dependency file with a grep + sed to add all the extra incbin statements as well, it's ugly, but it works in the common case." That's the case where -MC would be appropriate.

[ISSOtm]

I'm still not feeling happy about this, but eh.

[Rangi42]

I'm still not feeling happy about this, but eh.

If you're not happy about it because this implementation looks incorrect, incomplete, or inadequate for what users are wanting to accomplish, then I want to hear why and fix it before merge.

If it's just that you don't think this capability ought to be possible at all, I disagree. Users have repeatedly gotten tripped up and confused by how -MG currently aborts at the first not-yet-generated dependency. And gb-starter-kit demonstrates that writing a Makefile to adequately work with -MG's behavior, by somehow getting make to loop invocations until it's done, requires pretty advanced/subtle Makefile crafting.

I understand the point that sometimes continued processing would be incorrect, but the common case is that continued processing would be safe, and the docs do warn about that uncommon case.

From the comments in #903, it sounded like you were okay with a solution like this, but it just got lost/forgotten and we didn't implement it at the time:

How about just fixing the "failedOnMissingInclude causes YYACCEPT" issue for now, and once lazy evaluation exists, then do "-MG exits if an IF expression can't be evaluated"?

Worth trying, though I'd wait until #849 is done to have some testing.

Resolved

[ISSOtm]

gb-starter-kit provides a simple counter-example of what you call “common behaviour”:

INCLUDE "assets/file.pb16.size" ; ⚠️ Defines a constant.
ds NB_PB16_BLOCKS ; Fatal error: referencing non-const symbol in const context.

I disagree that this is truly “common behaviour”, and contend that both scenarii are equally as common, if not unbalanced in favour of the current solution.

[Rangi42]

gb-starter-kit provides a simple counter-example of what you call “common behaviour”:

INCLUDE "assets/file.pb16.size" ; ⚠️ Defines a constant.
ds NB_PB16_BLOCKS ; Fatal error: referencing non-const symbol in const context.

Not a counterexample to anything, that's just the uncommon behavior that I noted, which is why -MC is a new option and not the only possible behavior of -MG.

Is this not yet pushed? I see its Makefile support for defining NB_PB16_BLOCKS but nowhere it's actually used yet. Or is there a game using this .pb16.size file convention where I could see why the precompressed size needs to go in a ds? (libbet used .pb16 but for whatever reason doesn't use/need .size files.)

I disagree that this is truly “common behaviour”, and contend that both scenarii are equally as common, if not unbalanced in favour of the current solution.

You don't think simple generated INCBINs like in pokecrystal or libbet or ucity or LADX or little-things-gb are more common than... instances like these .size files where the generated content influences an if conditional or ds size? (Most sizes I see are of the post-generated data, where link-time-evaluated Data.End - Data works to get the size.)

[ISSOtm]

I don't contend that most uses of generated INCLUDE or INCBIN are “simple”, what I want to mean is that the existence of at least one “complex” include is more common than its lack across an entire project; therefore, -MC is not applicable globally to a project (and trying to apply it to a whitelist of files is cumbersome and negates much of the benefit, or trying to apply it except to a blacklist of files is error-prone and may cause frustrating “builds-on-my-machine-but-not-in-CI” situations).

[Rangi42]

True, projects shouldn't try to apply -MC to only some files; it's easier to go all or nothing. And maybe the projects that have no need for "abort on first" behavior are a minority.

I did test -MC for building polishedcrystal, by replacing use of tools/scan_includes with rgbasm -M (similarly to ISSOtm/gb-starter-kit#28):

--- a/Makefile
+++ b/Makefile
@@ -128,15 +128,17 @@ ifeq (,$(filter clean tidy tools,$(MAKECMDGOALS)))
 $(info $(shell $(MAKE) -C tools))
 endif
 
-preinclude_deps := includes.asm $(shell tools/scan_includes includes.asm)
-
 define DEP
-$1: $2 $$(shell tools/scan_includes $2) $(preinclude_deps) | rgbdscheck.o
+$(subst |,
+,$(shell $(RGBDS)rgbasm $(RGBASM_FLAGS) -M - -MG -MC -MQ $1 $2 | tr '\n' '|'))
+$1: $2 | rgbdscheck.o
        $Q$$(RGBDS)rgbasm $$(RGBASM_FLAGS) -o $$@ $$<
 endef
 
 define VCDEP
-$1: $2 $$(shell tools/scan_includes $2) $(preinclude_deps) | rgbdscheck.o
+$(subst |,
+,$(shell $(RGBDS)rgbasm $(RGBASM_VC_FLAGS) -M - -MG -MC -MQ $1 $2 | tr '\n' '|'))
+$1: $2 | rgbdscheck.o
        $Q$$(RGBDS)rgbasm $$(RGBASM_VC_FLAGS) -o $$@ $$<
 endef

The ROM builds identically as before, albeit more slowly since scan_includes was doing much less parsing work.

[ISSOtm]

If pret is the only current use case for this, but they'd decline to switch over due to the performance loss, is there an outlook for this functionality?

[Rangi42]

pret is not the only current use case; it's an example of a complex project where all of the dependencies are compatible with -MC. Presumably Liji and Daid also had use cases when they brought this up.

Even gb-starter-kit (and the multiple completed games based on it!) are compatible with -MC. Turns out that the .pb16.size dependencies do not influence subsequent compilation; NB_PB16_BLOCKS is not used in any ds, or if conditions, or so on. It's just lded.

(I'll admit I gave up on trying to build any of Shock Lobster, Esprit, or Rhythm Land as definite tests, because they're finicky in other ways -- hard-coded .exe extensions, Rust or Python package requirements, etc -- but if it's a blocker to agreeing on if/how -MC should work then I'll retry that.)

Thinking more about it, I'm actually pretty sure it's just plain bad practice, in any language, to make one generated dependency create more of them. In the general case, you end up with the same limitation as RGBASM -- what gets generated could do anything, so you have no choice but to halt the build and generate it before you can continue. But doing so in different build systems varies from "difficult" to "impossible", and usually languages aren't so flexible that they could possibly require such a thing anyway.

If there are any non-theoretical examples, then of course the devs can continue to use -MG without -MC, and ensure their Makefile (or whatever build system) builds things in the right order. But I don't know of such examples, and can't even come up with a plausible fake one. For example:

INCLUDE "generated-debug.inc" ; DEFs DEBUG to either 0 or 1
if DEBUG
  INCBIN "staging.bin"
else
  INCBIN "production.bin"
endc

Of course this would build incorrectly with -MC, but who is actually having a generated dependency like that? You'd either have plain debug.inc as a static file, or pass -DDEBUG=1 to rgbasm.

[Rangi42]

I expect that the gb-asm-tutorial would be another use case for -MG -MC. It starts out showing people how raw dw/db can make graphics, tilemaps, etc, but once it gets around to replacing those with INCBINs, -MG -MC will be the appropriately simple way to discover those dependencies.

(And if a later, more advanced step of the tutorial demonstrates generated dependencies more complex than foo.2bpp and bar.tilemap, where they somehow interact with if or ds or other asm-time constant values, then it can also explain how to remove -MC and how to adapt the Makefile to handle indefinite rgbasm -M looping. I think the interaction of .o and .mk and .dbg files in gb-starter-kit is quite complex enough to deserve an explanation anyway.)

[Rangi42]

An idea while reading the NASM docs (it has -M and does not have RGBASM's halt-on-first-missing-gen-dep behavior): we could have if include "..." and if incbin "..." to detect if the inclusion succeeded (and else branches if it did not). Then code that would break if the inclusion failed can prevent that, e.g. change this:

; the .size file would contain "DEF NB_PB8_BLOCKS EQU whatever"
INCLUDE "assets/crash_font.1bpp.pb8.size"
	ld c, NB_PB8_BLOCKS
	PURGE NB_PB8_BLOCKS

to this:

IF INCLUDE "assets/crash_font.1bpp.pb8.size"
	ld c, NB_PB8_BLOCKS
	PURGE NB_PB8_BLOCKS
ENDC

or even this, if you're worried about size differences:

IF INCLUDE "assets/crash_font.1bpp.pb8.size"
	ld c, NB_PB8_BLOCKS
	PURGE NB_PB8_BLOCKS
ELSE
	ld c, 0
ENDC

(Of course, projects that use things like .size files could also just stay as-is, without using -MC. But maybe some project has 90% safe incbins but a few that need to exist for subsequent assembly.)