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

Ninja build #231

Merged
merged 11 commits into from
Sep 14, 2022
Merged

Ninja build #231

merged 11 commits into from
Sep 14, 2022

Conversation

em-eight
Copy link
Collaborator

Use ninja for C and ASM compilation

@riptl
Copy link
Collaborator

riptl commented Sep 12, 2022

Nice work, finally.

@riptl
Copy link
Collaborator

riptl commented Sep 12, 2022

Few ideas for the future (might send PRs implementing those)

  • Rename the new build.py to configure.py, which is the conventional script name for a build file generator
  • Add always-out-of-date targets for helper scripts like ninja verify-symbols (invoking Python with a rule)
  • Use a machine-readable format for source lists and flags (e.g. YAML)
  • Integrate MWCC .d files to automatically detect include-only files as build dependencies
  • Integrate gen_asm into the Ninja build process
  • Give Ninja the ability to detect when build files need to be re-generated

With all of that is done, configure.py only has to be invoked once directly during setup.

Any other interactions with the build system would be accessible through the ninja command, which has a few benefits

  • Seamless integrations with IDEs that support Ninja (VSCode)
  • Faster builds when asm needs to be regenerated
  • Better to debug and maintain

Probably too overkill and idealistic for a decomp project but it could help build a standard template

@SeekyCt
Copy link
Contributor

SeekyCt commented Sep 12, 2022

MWCC .d files don't work on linux, but dkp's cpp is used instead and should be able to do everything that they do already afaik

diff_settings.py Outdated
@@ -17,7 +17,7 @@ def apply(config: dict, args):
config["myimg"] = "artifacts/target/pal/main.dol"
config["baseimg"] = "artifacts/orig/pal/main.dol"
config["symbols_yml"] = "pack/symbols.yml"
config["make_command"] = [executable, "build.py", "--diff_py"]
config["make_command"] = "ninja"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be ["ninja"] btw

@em-eight
Copy link
Collaborator Author

em-eight commented Sep 12, 2022

All the suggestions are reasonable. One thing to note is that the main remaining performance gain is by by quite a margin assembly regeneration, but we need to figure out and implement our own depfile. After that, ninja integration is pretty straightforward.

@SeekyCt
Copy link
Contributor

SeekyCt commented Sep 12, 2022

Does it need anything more complicated than just depending on the slices and symbols files?

@em-eight
Copy link
Collaborator Author

The best thing would be to actually read them and generate only what's neccessary, it would save a lot of time.

@em-eight
Copy link
Collaborator Author

Btw, give me an hour or two before merging to add some of what terorie mentioned

@SeekyCt
Copy link
Contributor

SeekyCt commented Sep 12, 2022

The best thing would be to actually read them and generate only what's neccessary, it would save a lot of time.

I think that's something you'd have to do at the tool level rather than the ninja level though?

@em-eight
Copy link
Collaborator Author

The best thing would be to actually read them and generate only what's neccessary, it would save a lot of time.

I think that's something you'd have to do at the tool level rather than the ninja level though?

Yes, similar to how we use dkp gcc as a script to generate a depfile for C

@em-eight
Copy link
Collaborator Author

Note that the tools only generate the depfile, not the asm itself

@riptl
Copy link
Collaborator

riptl commented Sep 12, 2022

The best thing would be to actually read them and generate only what's neccessary, it would save a lot of time.

I think that's something you'd have to do at the tool level rather than the ninja level though?

If we want to do it in Ninja, it requires one level of indirection.
We could model a rule that generates a intermediate-step build.ninja from slices and symbols.txt.

For slices, this is easy – if the range changes, the resulting asm file name will also change and be rebuilt.

Reacting to symbol changes is a bit more difficult. symbols.txt is spans lots of asm files. So if the asm gen rule directly depends on it directly, it would regen everything if the modification date changes (e.g. a single line is added).
One workaround is to introduce a tool that splits symbols.txt into a file per object, which is cheap enough.
Then, let the object targets depend on their respective symbols split file.

Unlike Make, Ninja doesn't natively support recursion, so it would be hacky or require a wrapper script anyways.
It's probably not worth the effort.

@em-eight
Copy link
Collaborator Author

Probably easier to do at the tool level and reconfigure ninja yeah

@em-eight
Copy link
Collaborator Author

@SeekyCt Aight, PR is ready, I need some help with the windows CI though...

@SeekyCt
Copy link
Contributor

SeekyCt commented Sep 12, 2022

It doesn't seem to like -Isource/**

@em-eight
Copy link
Collaborator Author

Perhaps there is some way to glob on windows? Maybe with a userspace utility?

@em-eight
Copy link
Collaborator Author

@SeekyCt
Copy link
Contributor

SeekyCt commented Sep 12, 2022

Is there anything wrong with just doing source and source/platform like the CW args do?

@em-eight
Copy link
Collaborator Author

You're right

@em-eight
Copy link
Collaborator Author

cc1.exe: error: bad value ('750') for '-mtune=' switch
what

@em-eight
Copy link
Collaborator Author

@terorie @riidefi Just in case you didn't notice discord, the PR is ready for merge

@riidefi
Copy link
Owner

riidefi commented Sep 14, 2022

LGTM. Nice work!

@riidefi riidefi merged commit 90979ca into riidefi:master Sep 14, 2022
riidefi pushed a commit that referenced this pull request Feb 25, 2024
* Ninja build for C/ASM sources

* ninja on linux

* Remove old build flags

* Update CI and diff settings

* Change links to dropbox

* Add link rules for ninja

* Add rules for packing and verifying, update README and CI

* allow chain

* gcc depfile include dirs

* nostdinc

* Different tools installation

Co-authored-by: Theodoros Tyrovouzis <teotyrov@gmail.com>
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.

5 participants