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

apps: use CONFIGURED_APPS to iterate clean targets #397

Closed
wants to merge 1 commit into from
Closed

apps: use CONFIGURED_APPS to iterate clean targets #397

wants to merge 1 commit into from

Conversation

protobits
Copy link
Contributor

Summary

Before this change CLEANDIRS was used, which is built from
detecting dummy .depend/.kconfig files, which is problematic
since when there's an intermediate subdirectory which does not
have it, subdirectories below this level will not be found
(even if they do have these files).

Since we already know exactly which subdirectories we need
to clean based on configured apps, we directly clean over these.

I started looking into this since I found that the btsak application
was not being cleaned due to this reason, which sits at apps/wireless/bluetooth/btsak
and bluetooth was not being cleaned since there were none of this dummy files there.

I would like to hear your thoughts on why this might be wrong
or not. I feel there's quite a bit of "magic files" placed around and
these are used to drive many targets and this is becoming very confusing
to maintain. My reasoning is that we should go into the subdirectories
we know need cleaning from the top-level Makefile directly and then let
each application Makefile drive the cleaning using its clean target.

I have not changed yet the similar logic in Directory.mk for CLEANSUBDIRS
which is still driven by .depend and .kconfig files as it is not clear to me
how works. I'm guessing that after this change, it is not necessary, but I'm unsure.

Impact

Application build system

Testing

Building and cleaning and looking for leftover .os

Before this change CLEANDIRS was used, which is built from
detecting dummy .depend/.kconfig files, which is problematic
since when there's an intermediate subdirectory which does not
have it, subdirectories below this level will not be found
(even if they do have these files).

Since we already know exactly which subdirectories we need
to clean based on configured apps, we directly clean over these.
@protobits
Copy link
Contributor Author

Note: an alternative approach would be to solve this recursively: have the top-level Makefile go into one level below, clean there, recurse into subdirectories. I tried this (and I believe this is how it was originally) but this is really slow because it goes into absolutely every subdirectory, even when we know it was not built. Thus my reasoning of directly going into configured application directories.

@protobits protobits marked this pull request as draft September 19, 2020 15:32
@protobits
Copy link
Contributor Author

Actually I believe the clean logic is Directory.mk is unnecessary since it is designed following the recursive approach. I removed the clean target there and all .o's were still removed.

@protobits
Copy link
Contributor Author

One final note (sorry for spamming with comments): I understand this may leave .o present if you for example build, configure an app out and then clean. But I think that: a) this should never be a problem to build with extra .os present as all the logic looks for specific .os, not just any .o in a directory, and b) if one wants a pristine directory it should do distclean. In this case distclean should indeed go through every subdirectory to ensure a clean workspace. But that's at least my thinking.

@protobits
Copy link
Contributor Author

I see this is problematic because we need to clean autogenerated Kconfig files in intermediate directories. I'm going to try another approach: to use CONFIGURED_APPS to do go through all intermediate subdirectories of CONFIGURED_APPS only

@protobits
Copy link
Contributor Author

I'm closing this since since I explained the problem in the linked issue but I don't yet have a successful solution

@protobits protobits closed this Sep 19, 2020
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.

None yet

1 participant