Skip to content

Avoid creating intermediate .mk files #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

Rangi42
Copy link

@Rangi42 Rangi42 commented May 6, 2025

My goal here is to make issue #1 a moot point, since no file timestamps will be involved.

Instead of storing the rgbasm -M output in a .mk file for each .o and then includeing all those .mk files in the Makefile, this just directly dumps the rgbasm -M output into the Makefile.

This is similar to how pokecrystal's Makefile works, using scan_includes instead of rgbasm -M.

It has to work around the fact that $(shell) unavoidably turns newlines into spaces, by tring them into pipe characters and then substituting to undo the transformation. (The pipe | is highly unlikely to be in any Linux filenames, and is outright forbidden on Windows.)

I did have an issue with building this from scratch on macOS, where make would fail (Error opening INCBIN file 'assets/crash_font.1bpp.pb8': No such file or directory) but running it again would succeed. This was because rgbasm -M was not outputting the assets/crash_font.1bpp.pb8 target unless assets/crash_font.1bpp.pb8.size existed. I think that's a bug with RGBDS; see gbdev/rgbds#903 (comment). Anyway, having .pb8.size depend on .pb8 directly fixes it too.

Copy link
Owner

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work for nested dependencies, nor RGBDS if there are two generated files necessary in a row. RGBASM needs to be run in a loop until the object file exists.

@Rangi42
Copy link
Author

Rangi42 commented May 6, 2025

How does the previous design avoid the need to run rgbasm in a loop?

Would resolving gbdev/rgbds#903 (comment) be sufficient?

As for nested dependencies, I don't see how the current design handles them either. Every .asm file gets a .o+.mk file, and only .inc files are supposed to be INCLUDEd. If foo.inc then has INCLUDE "bar.inc", when/how would it ever run rgbasm -MG bar.inc?

@ISSOtm
Copy link
Owner

ISSOtm commented May 7, 2025

How does the previous design avoid the need to run rgbasm in a loop?

It doesn't:

[...] this will cause Make to “reload” its dependency information from the modified .mk file, (try to) make the newly discovered dependencies, and then re-run RGBASM, until the .o file is successfully created!

That behaviour is built into Make.

Would resolving gbdev/rgbds#903 (comment) be sufficient?

No. You need to re-build the build graph after discovering new dependencies.

As for nested dependencies, I don't see how the current design handles them either. Every .asm file gets a .o+.mk file, and only .inc files are supposed to be INCLUDEd. If foo.inc then has INCLUDE "bar.inc", when/how would it ever run rgbasm -MG bar.inc?

It wouldn't; instead, it would re-run rgbasm -MG foo.asm, and transitively discover the dependencies of bar.inc via foo.asm's INCLUDE "bar.inc".

@Rangi42
Copy link
Author

Rangi42 commented May 7, 2025

Thanks for running the CI. It does at least now build on Windows, and on macOS without having to brew install make, so shall we leave this PR open as a draft? If I can ensure that it handles (1) nested deps and (2) multiple nonexistent deps in a row, it would be a a functional replacement here.

@Rangi42 Rangi42 marked this pull request as draft May 7, 2025 03:06
@ISSOtm
Copy link
Owner

ISSOtm commented May 7, 2025

CI passes with this change because the default project only has a single generated dependency at all; if a more complex project (I could think of Shock Lobster, Esprit, and Rhythm Land) can build successfully with this change, then it would be appropriate to merge.

@Rangi42
Copy link
Author

Rangi42 commented May 8, 2025

I don't see a way to avoid the need for running rgbasm -M in a loop. Not every project can use -MC, and even the ones that can might have nested recursive dependencies (e.g. INCLUDE "this-needs-generating.inc" where this-needs-generating.inc will have INCBIN "this-also-needs-generating.2bpp"). And if it has to be run in a loop, using make's change detection for the .mk file is a clean way to do it (apart from the macOS and Windows issues).

@Rangi42 Rangi42 closed this May 8, 2025
@Rangi42 Rangi42 deleted the deps branch May 8, 2025 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants