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: simplify; clean should remove images created #42

Merged
merged 1 commit into from
Nov 14, 2022
Merged

Makefile: simplify; clean should remove images created #42

merged 1 commit into from
Nov 14, 2022

Conversation

allanwind
Copy link
Contributor

@allanwind allanwind commented Nov 1, 2022

The target all was previous running setup followed by build. As both setup and build are phony targets they run in sequence and because the firmware contain the timestamp they always run (dependencies doesn't do anything). all now does exactly what it says on the tin without introducing non-standard targets.

clean should remove the images that created during build which is what you expect from that target. See:
https://www.gnu.org/software/make/manual/html_node/Standard-Targets.html#Standard-Targets

The target all was previous running setup followed by build.  As
both setup and build are phony targets they run in sequence and because
the firmware contain the timestamp they always run (dependencies doesn't
do anything).  all now does exactly what it says on the tin
without introducing non-standard targets.

clean now removes the images that created during build which is
what you expect from that target.  See:
https://www.gnu.org/software/make/manual/html_node/Standard-Targets.html#Standard-Targets
@allanwind
Copy link
Contributor Author

allanwind commented Nov 1, 2022

Tested with podman. Please test the clean target on docker.

@allanwind allanwind changed the title Makefile: simplify; clean removes images Makefile: simplify; clean should remove images created Nov 1, 2022
@allanwind
Copy link
Contributor Author

The next step (for others to pursue) would be to build a fixed target (say, without the timestamp- prefix) so the build step only happens if the dependencies change. I haven't analyzed what those are yet but that should be complete to be reliable; not sure how that would work with the zmk dependency if you can infer that from $(DOCKER) build .., $(DOCKER) run ... or if one would need to mount a file from the container onto the source tree. A cold build (to create the zmk container) takes 4m56s for me , rebuild (with no changes) 53s with the goal being ~0s.

Why can't everything be upstreamed to zmk so this just repo would just be a fork?

@ReFil
Copy link
Collaborator

ReFil commented Nov 14, 2022

Sorry for the radio silence lately, been very busy. The adv360 has several unique features that are in the process of getting upstreamed into zmk main however this isn't an instant process. The local building from a config repo is unique amongst boards powered by ZMK so upstreaming this config repo stuff wouldn't be trivial. looking at this it seems to be changing a lot more than it needs to to remove the docker image, why was that done?

@allanwind
Copy link
Contributor Author

allanwind commented Nov 14, 2022 via email

@ReFil
Copy link
Collaborator

ReFil commented Nov 14, 2022

Thanks for explaining it all to me, makes sense now :)

@ReFil ReFil merged commit 2562f62 into KinesisCorporation:V2.0 Nov 14, 2022
@adammodlin
Copy link

I commented on the merge but will put here. Can you update the readme since it's now outdated?

@ReFil
Copy link
Collaborator

ReFil commented Nov 16, 2022

I commented on the merge but will put here. Can you update the readme since it's now outdated?

#57 should take care of this

@allanwind allanwind deleted the makefile branch December 23, 2022 05:41
tricktux pushed a commit to tricktux/Adv360-Pro-ZMK that referenced this pull request Feb 10, 2024
Makefile: simplify; clean should remove images created
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.

3 participants