search

OoTRandomizer / OoT-Randomizer

A randomizer for Ocarina of Time.
Other
397 stars 230 forks source link

Fix order in Makefile #2243

Open flagrama opened 3 weeks ago

flagrama commented 3 weeks ago

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

jdunn596 commented 3 weeks ago

Can confirm this fixes the issue

fenhl commented 1 week ago

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.

jdunn596 commented 1 week ago

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

fenhl commented 1 week ago

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”).

jdunn596 commented 1 week ago

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.

flagrama commented 1 week ago

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.