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

make help: show generate-Makefile.ci #20186

Merged
merged 1 commit into from
Jan 4, 2024
Merged

Conversation

mguetschow
Copy link
Contributor

Contribution description

generate-Makefile.ci is the only target interesting to the user containing uppercase and a dot, which wasn't matched by the sed command used in make help. Changing that accordingly matches *.module and *.xml targets, too, which still shouldn't be part of the user-visible targets. Therefore, those are filtered out with grep.

Testing procedure

  1. on master: make -C examples/hello-world help > before
  2. with this PR: make -C examples/hello-world help > after
  3. diff before after results in:
46a47
> generate-Makefile.ci

Issues/PRs references

generate-Makefile.ci was added with #19244

@github-actions github-actions bot added the Area: build system Area: Build system label Dec 15, 2023
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 15, 2023
@benpicco benpicco requested review from maribu and bergzand and removed request for maribu December 15, 2023 10:12
Makefile.include Outdated
@@ -1031,7 +1031,7 @@ ifneq (, $(filter all flash, $(if $(MAKECMDGOALS), $(MAKECMDGOALS), all)))
endif

help:
@$(MAKE) -qp | sed -ne 's/\(^[a-z][a-z_-]*\):.*/\1/p' | sort | uniq
@$(MAKE) -qp | sed -ne 's/\(^[a-z][a-zA-Z._-]*\):.*/\1/p' | grep -Fv -e '.module' -e '.xml' | sort | uniq
Copy link
Contributor

@benpicco benpicco Dec 15, 2023

Choose a reason for hiding this comment

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

Please add a comment as to what this incantation is good for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added and force-pushed.

Copy link
Contributor

@kaspar030 kaspar030 Dec 15, 2023

Choose a reason for hiding this comment

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

you could move the grep as additional -e expression to sed. not sure that makes it more readable or measurably faster.

Suggested change
@$(MAKE) -qp | sed -ne 's/\(^[a-z][a-zA-Z._-]*\):.*/\1/p' | grep -Fv -e '.module' -e '.xml' | sort | uniq
@$(MAKE) -qp | sed -e '/\.module$/d' -e '/\.xml$/d' -ne 's/\(^[a-z][a-zA-Z._-]*\):.*/\1/p' | sort | uniq

My sort has an -u flag that further allows removing uniq.

@riot-ci
Copy link

riot-ci commented Dec 15, 2023

Murdock results

✔️ PASSED

292393c make help: show generate-Makefile.ci

Success Failures Total Runtime
8082 0 8082 10m:47s

Artifacts

@mguetschow
Copy link
Contributor Author

Further thinking about it after @kaspar030's comment lead me to reconsider my choice of blacklisting specifically *.module and *.xml, I could imagine there to be more non-user targets for different configurations/applications. Maybe a whitelisting approach to include generate-Makefile.ci explicitely is a better idea?

Something like sed -ne 's/\(^[a-z][a-z_-]*\|generate-Makefile.ci\):.*/\1/p'

@benpicco
Copy link
Contributor

benpicco commented Dec 15, 2023

Ideally we would have a map: target - description

The target names alone are of little help unless you already know what they are doing.

generate-Makefile.ci is the only target interesting to the user containing uppercase and a dot.
Instead of matching all such targets with sed, generate-Makefile.ci is matched explicitly, to avoid showing internal targets.
@mguetschow
Copy link
Contributor Author

Maybe a whitelisting approach to include generate-Makefile.ci explicitely is a better idea?

That's what I've changed it to now. Still feels a bit hacky, but probably cleaner than the former solution.

Ideally we would have a map: target - description

Very true, I second this! But until that is added, this PR still is an improvement over the current situation.

The target names alone are of little help unless you already know what they are doing.

In this case I remembered there was a Makefile command for generating the Makefile.ci, but couldn't find it in the list.

@benpicco benpicco added this pull request to the merge queue Jan 3, 2024
@benpicco benpicco removed this pull request from the merge queue due to a manual request Jan 3, 2024
@benpicco benpicco added this pull request to the merge queue Jan 3, 2024
Merged via the queue into RIOT-OS:master with commit 4890eff Jan 4, 2024
26 checks passed
@mguetschow mguetschow deleted the make-help branch January 5, 2024 09:16
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants