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

Makefile overhaul #666

Merged
merged 55 commits into from
Aug 27, 2016
Merged

Makefile overhaul #666

merged 55 commits into from
Aug 27, 2016

Conversation

fredizzimo
Copy link
Contributor

Here comes a scary pull request(it turned out to be much bigger than I had anticipated)... I recommend that we let it rest a little bit for proper review and/or testing by several people before merging.

I have been testing it quite much myself, but the magnitude of the changes are huge, and I hope I have been taken care of everything.

The readme.md file best explains the new syntax for running make.

The biggest reason for the huge number of files changed is that keyboard and subproject makefiles have been split into Makefile and rules.mk. The documentation also had to be updated in many files. Other than that the changes are quite localized.

The individual commit descriptions best describe what have been made. But here's a quick overview of what has been done from issue #591, (1. and 2. and 9. were fixed before). 3, 4, 5, 7 (partly), 8 (but keymaps still use only a single Makefile).

Additionally there are a couple of optimizations.
#582, or keep going even after a keyboard compilation fails is now on by default (it doesn't check the -c option). I figured it was better to do without, as you can always press Ctrl-c if you want to abort the compilation in the middle.

There's also an unrelated change, since my previous make file changes, diff-utils have been needed for the cmp command. So that have now been added to Travis and install-dependencies.sh. The instructions have not been updated since they seemed to be incomplete even before, and needs a separate update pass.

The root Makefile, could perhaps benefit from a bit more comments, an I will probably add that tomorrow.

@fredizzimo
Copy link
Contributor Author

Seems like I forgot the new amj60 keyboard.

When running make from either a keyboard folder or a subproject
it runs all keymaps for all subprojects and the selected subproject
respectively. Without this fix, the same doesn't happen if your
run make clean for example. As it would just provide you with an
error message. Now this will work as expected.
@fredizzimo
Copy link
Contributor Author

I fixed a small inconsistency issue.

When running make from either a keyboard folder or a subproject it builds all keymaps for all subprojects and the selected subproject respectively. Without this fix, the same doesn't happen if you run make clean for example. As it would just provide you with an error message. Now this will work as expected.

@fredizzimo
Copy link
Contributor Author

Closing and re-opening to trigger Travis.

@fredizzimo fredizzimo closed this Aug 24, 2016
@fredizzimo fredizzimo reopened this Aug 24, 2016
It had been added for some strange reason, allthough it's supposed
to be there only in another branch.
@fredizzimo
Copy link
Contributor Author

fredizzimo commented Aug 25, 2016

This pull request is getting a bit hard to keep up to date and I will soon have the unit testing pull request, which builds on this, ready. So it would be good if it could be merged soon.

There are some line ending inconsistencies (most likely caused by the .gitattributes changes that prevents me from rebasing properly, so the last two updates had to be done through merges. But unfortunately it also means that any change made to a keyboard or subproject makefile won't propagate to the rules.mk file anymore. And I have to resolve that manually, so it's very error prone.

Personally I haven't found any problems with this.
@algernon, Did you have a chance to test my fix already, and could you please confirm that it works?
@jackhumbert Have you been able to test this at all. Do you have any comments or improvements that should be done before merging?

One quick way to test different make commands is to use the show_path target, which doesn't compile, but still shows what would be done, so it's much faster that way.

Edit:
Yes, it's really error prone to do merges, as I already broke it...

Led should have been added to KC60, not GH60
@jackhumbert
Copy link
Member

Agreed! I'd like to merge this soon - I'll try to set aside some time today to dive into it. It's been a while since I've been able to pull/compile locally :)

@sethbc
Copy link
Contributor

sethbc commented Aug 27, 2016

FWIW, this runs okay for me. i'd probably add an alias to clean all ("make clean"?)

@fredizzimo
Copy link
Contributor Author

fredizzimo commented Aug 27, 2016

Yes, adding a make clean from the root would be a good idea.

I think I'll implement it so that it just deletes the whole .build folder, instead of calling each target sepearately, which make allkb-allsp-allkb-clean would do. That should speed up the cleaning considerably.

Edit: I'll make that in a separate pull request, in order to not keep adding features to this one

@jackhumbert
Copy link
Member

Messing around with this a bit, and I'm comfortable merging it! The only thing I might suggest is a make help for those too lazy to look at the documentation :)

My current understanding of the order of merging is #666, #691, and then #690 once you're comfortable with it - is that accurate?

@fredizzimo
Copy link
Contributor Author

Yes, the order is correct. #691 will probably break after the merge of #666, but I will fix it if it does. #690 (Unit testing) will be rebased on top of the other merges.

@fredizzimo
Copy link
Contributor Author

Hm When working on the unit test compilation, I just noticed that compiled_push doesn't work.

1.14s$ bash util/travis_compiled_push.sh
Making ergodox/ez with keymap steno and target all-keymaps
build_keyboard.mk:44: *** "keyboards/ergodox/ez/ergodox/ez.c" does not exist.  Stop.
util/travis_compiled_push.sh: line 14: GH_TOKEN: unbound variable

So this probably have to be fixed before merging.

@jackhumbert Why is it trying to compile the Ergodox keyboards again there?

@jackhumbert
Copy link
Member

There's a line (make all-keymaps keyboard=ergodox/ez AUTOGEN=true) in the script that recompiles the EZ files, so those get used on qmk.fm instead of the infinity ones. It's a temporary fix until I'm able to implement subprojects into the qmk.fm generation.

make all-keymaps keyboard=ergodox/ez AUTOGEN=true should be able to be replaced with make keyboard=ergodox AUTOGEN=true, right?

@fredizzimo
Copy link
Contributor Author

Yes, that should work, but I changed it to the more simple form
make ergodox-ez AUTOGEN=true,

make ergodox would compile both EZ and Infinity.

@fredizzimo
Copy link
Contributor Author

Everything seems to be OK now, I assume that the util/travis_compiled_push.sh: line 14: GH_TOKEN: unbound variable, is caused by not starting the compilation from the master branch. And that's what currently prevents pull requests from updating the qmk.fm.

@jackhumbert
Copy link
Member

Awesome! Yeah, I really got lucky there with avoiding that issue :) Merging now!

@jackhumbert jackhumbert merged commit 36b6a96 into qmk:master Aug 27, 2016
@fredizzimo fredizzimo deleted the makefile_overhaul branch June 18, 2017 11:54
BlueTufa pushed a commit to BlueTufa/qmk_firmware that referenced this pull request Aug 6, 2021
rti pushed a commit to rti/qmk_firmware that referenced this pull request Feb 25, 2024
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.

4 participants