OoT-Randomizer icon indicating copy to clipboard operation
OoT-Randomizer copied to clipboard

Fix order in Makefile

Open flagrama opened this issue 1 year ago • 6 comments

This should solve the issue where if the build/bin directory doesn't exist the build will fail.

flagrama avatar Jun 09 '24 18:06 flagrama

Can confirm this fixes the issue

jdunn596 avatar Jun 09 '24 18:06 jdunn596

Can anyone explain why this fixes the issue? I've been staring at this file for a while now and I still don't understand how this could affect anything.

fenhl avatar Jun 20 '24 00:06 fenhl

I have no idea why it works since I don't have any experience with makefiles but I did test the effect of the change in a clean enviornment and it creates the missing directory as intended

Maybe prereqs need to be listed in the order they are defined? I have no idea

jdunn596 avatar Jun 20 '24 00:06 jdunn596

I understand that and that's why the “needs testing” label has been removed. Code review still needs to happen separately from that, and with something like a load-bearing dependency order, there really ought to at least be a comment warning about it (even if that comment says “we have no idea why but these deps need to be in this order, otherwise you get x error if build/bin doesn't exist”).

fenhl avatar Jun 20 '24 00:06 fenhl

After some research, it currently fails because $(RESOURCES) are being built first, which depend on OBJDIR (indicated in line 20). At this point, $(OBJECTS): | $(OBJDIR) hasn't been built, which means build/bin hasn't been created either (line 33). Reordering the deps in bundle and building $(OBJECTS) beforehand ensures that $(OBJDIR) is built and the directory exists before trying to place resources in the defined location.

jdunn596 avatar Jun 20 '24 01:06 jdunn596

It might make more sense to have the OJBDIR and RESOURCEDIR be dependencies of bundle as well before OBJECTS and RESOURCES rather than relying on the OBJECTS and RESOURCES dependencies being in the right order.

flagrama avatar Jun 20 '24 15:06 flagrama

Okay, I think I see the problem now: RESOURCES depends on OBJDIR but this isn't declared as a $(RESOURCES): | $(OBJDIR) dependency. In that case, I think a better fix would be to add that line to make the dependency explicit instead of reordering things.

fenhl avatar Aug 12 '24 19:08 fenhl