Skip to content
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

Many smaller .o files, kept in their own build directory #425

Open
roukaour opened this issue Dec 25, 2017 · 14 comments
Open

Many smaller .o files, kept in their own build directory #425

roukaour opened this issue Dec 25, 2017 · 14 comments
Assignees

Comments

@roukaour
Copy link
Contributor

e.g. objects/ or temp/

Probably there are also more self-contained subsystems that can get their own .o files to improve build time.

@yenatch
Copy link
Contributor

yenatch commented Dec 25, 2017

build/pokecrystal/ and build/pokecrystal11/, or just build/

@roukaour
Copy link
Contributor Author

Note that if .o files aren't next to corresponding .asm files, this will no longer work:

$(foreach obj, $(crystal11_obj), $(eval $(call DEP,$(obj),$(obj:11.o=.asm))))
$(foreach obj, $(crystal_obj), $(eval $(call DEP,$(obj),$(obj:.o=.asm))))

@yenatch
Copy link
Contributor

yenatch commented Dec 25, 2017

i imagine it would work the same as pokeruby where subdirs are created in build/pokecrystal/

@yenatch yenatch closed this as completed Dec 25, 2017
@yenatch yenatch reopened this Dec 25, 2017
@yenatch
Copy link
Contributor

yenatch commented Dec 25, 2017

or build/crystal//build/crystal11/

@mid-kid
Copy link
Member

mid-kid commented Jan 22, 2018

If we could create all build output in a build/ directory (i.e. gfx files), that'd be great.

@Rangi42
Copy link
Member

Rangi42 commented Jan 23, 2018

What if the code mentioned a build directory, e.g.:

INCBIN "build/gfx/player/chris_back.2bpp.lz"

And then the Makefile rules were changed to map build/X to X? Like:

build/%.2bpp: %.png
	$(RGBGFX) $(rgbgfx) -o $@ $<
	$(if $(tools/gfx),\
		tools/gfx $(tools/gfx) -o $@ $@)

It would also need @mkdir -p $(dirname $^) since build/gfx/player/ doesn't exist yet.

This has the advantage of the current system, that you don't need an explicit list of built files and their dependencies, plus the advantage of not cluttering source directories with build products.

@yenatch
Copy link
Contributor

yenatch commented Jan 23, 2018

mkdir -p in recipes would dramatically slow down the build. doing it prebuild works fine though

the disadvantage is it's confusing, but i guess it already is confusing to include a different file than you put in

@Rangi42
Copy link
Member

Rangi42 commented Jun 6, 2018

pokegold-spaceworld has a working solution for this.

@Rangi42 Rangi42 changed the title .o files should be created in their own directory Many smaller .o files, kept in their own build directory Apr 14, 2019
@rawr51919
Copy link
Contributor

Could said solution be adapted to work here too? Surely it wouldn't take too much effort.

@mid-kid
Copy link
Member

mid-kid commented Jan 5, 2020

No, because everything is in monolithic sections and INCLUDEing all of the constants in every single build unit is extremely slow.
See #631

@Rangi42 Rangi42 linked a pull request Apr 7, 2020 that will close this issue
@rawr51919
Copy link
Contributor

What if the code mentioned a build directory, e.g.:

INCBIN "build/gfx/player/chris_back.2bpp.lz"

And then the Makefile rules were changed to map build/X to X? Like:

build/%.2bpp: %.png
	$(RGBGFX) $(rgbgfx) -o $@ $<
	$(if $(tools/gfx),\
		tools/gfx $(tools/gfx) -o $@ $@)

It would also need @mkdir -p $(dirname $^) since build/gfx/player/ doesn't exist yet.

This has the advantage of the current system, that you don't need an explicit list of built files and their dependencies, plus the advantage of not cluttering source directories with build products.

This solution might work much better and not be so damn massive of a refactor compared to mid-kid's work, perhaps doing the mkdir on the entire build directory structure before starting the build might be the way to go with this one as yenatch once specified. When I have time I could make a PR for this specific method

@vulcandth
Copy link
Collaborator

No, because everything is in monolithic sections and INCLUDEing all of the constants in every single build unit is extremely slow. See #631

Sorry to necro this topic... Is this due to how make does it's dependency checking? Would gbdev/rgbds#1043 help in this case?

@mid-kid
Copy link
Member

mid-kid commented Sep 22, 2022

It wouldn't be very different than adding INCLUDE "everything.inc" at the start of every file. In splitting I had reformatted every file to only INCLUDE the files it needed directly, and some included files would include other files recursively depending on what they needed (for example the many files that need macros/enum.inc). It used include guards to prevent double includes, just like in C.

Back then doing it this way gave at least a 4x speedup in clean rebuilds over the INCLUDE "everything.inc" solution. However, rgbds has changed a lot over the years since, and has gotten significant improvements in terms of performance, so this should be re-evaluated. Personally I still believe including files individually has benefits over everything.inc in preventing name conflicts. For example, you don't need the map script macros or pokemon and move constants in audio scripts, and vice versa. I had an audio_common.inc and events_common.inc for both of these kinds of scripts, so maybe there's a balance to be struck, but that's a conversation for a different day.

As an aside, preinclude is a nice feature but I almost prefer having an INCLUDE "everything.inc" in every file, even if only to cover the cases where you specifically don't want everything to be included, without having to modify the makefile.

@Rangi42
Copy link
Member

Rangi42 commented Sep 22, 2022

I think --preinclude is worth considering for the current architecture where we just have a few top-level .o files listed in the Makefile, but with splitting, yeah, manual includes are more clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants