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

build: improve readability of Makefile.am & the respective subdir.am #17516

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ariel-anieli
Copy link

@ariel-anieli ariel-anieli commented Nov 26, 2024

Why?

Hello committers,

Thanks for maintaining the project; this is my first (up to date) contribution. I would like to understand how are implemented routing protocols.

This PR improves the readability of Makefile.am.

Thank you,

How?

@ariel-anieli
Copy link
Author

Hello; any news on the PR?

@donaldsharp
Copy link
Member

@eqvinox can you take a look at this?

These changes are in relation to some bitcode compilation work done by @eqvinox several years back. In all honesty the only person who understands this bit of code is them. Let's see if we can get a review.

I just completely removed the SUFFIXES and the rules and everything built properly for me. So I have no idea what is even needed here at all.

@ariel-anieli
Copy link
Author

ariel-anieli commented Dec 4, 2024

Thanks for having taken a look, @donaldsharp; indeed, these extensions do appear only once in the repo. They were introduced for LLVM IR compilation (8fb4037).

# git grep -P '\.l?o\.bc'
Makefile.am:SUFFIXES += .lo.bc .o.bc
Makefile.am:.o.o.bc:
Makefile.am:.lo.lo.bc:

@eqvinox, does the change make sense? Let me know, I will push subsequent PRs. I spotted other places wherein SUFFIXES could be removed: the rules could be more legible.

# git grep -n SUFFIXES
Makefile.am:132:SUFFIXES =
Makefile.am:159:SUFFIXES += .proto .pb-c.c .pb-c.h
Makefile.am:307:SUFFIXES += .lo.bc .o.bc
babeld/Makefile:10:.SUFFIXES:
bfdd/Makefile:10:.SUFFIXES:
bgpd/Makefile:10:.SUFFIXES:
bgpd/rfp-example/librfp/Makefile:10:.SUFFIXES:
bgpd/rfp-example/rfptest/Makefile:10:.SUFFIXES:
doc/Makefile:18:.SUFFIXES:
doc/developer/Makefile:16:.SUFFIXES:
doc/manpages/Makefile:12:.SUFFIXES:
doc/user/Makefile:16:.SUFFIXES:
eigrpd/Makefile:10:.SUFFIXES:
fpm/Makefile:10:.SUFFIXES:
grpc/Makefile:10:.SUFFIXES:
grpc/subdir.am:33:SUFFIXES += .pb.h .pb.cc .grpc.pb.cc
isisd/Makefile:10:.SUFFIXES:
ldpd/Makefile:10:.SUFFIXES:
lib/Makefile:10:.SUFFIXES:
lib/subdir.am:490:SUFFIXES += _clippy.c
lib/subdir.am:503:SUFFIXES += .xref
mgmtd/Makefile:10:.SUFFIXES:
nhrpd/Makefile:10:.SUFFIXES:
ospf6d/Makefile:10:.SUFFIXES:
ospfclient/Makefile:10:.SUFFIXES:
ospfd/Makefile:10:.SUFFIXES:
pathd/Makefile:10:.SUFFIXES:
pbrd/Makefile:10:.SUFFIXES:
pimd/Makefile:10:.SUFFIXES:
qpb/Makefile:10:.SUFFIXES:
ripd/Makefile:10:.SUFFIXES:
ripngd/Makefile:10:.SUFFIXES:
sharpd/Makefile:10:.SUFFIXES:
staticd/Makefile:10:.SUFFIXES:
tests/Makefile:10:.SUFFIXES:
tools/Makefile:10:.SUFFIXES:
vrrpd/Makefile:10:.SUFFIXES:
vtysh/Makefile:10:.SUFFIXES:
watchfrr/Makefile:10:.SUFFIXES:
yang/subdir.am:1:SUFFIXES += .yang .yang.c .yin .yin.c
zebra/Makefile:10:.SUFFIXES:

# git grep -Pn '^\.[a-z_]+([a-z\._])+:' 
Makefile.am:309:.o.o.bc:
Makefile.am:311:.lo.lo.bc:
grpc/subdir.am:42:.proto.pb.cc:
grpc/subdir.am:44:.proto.grpc.pb.cc:
lib/subdir.am:491:.c_clippy.c:
lib/subdir.am:543:.l.c:
lib/subdir.am:545:.y.c:
yang/subdir.am:4:.yang.yang.c:
yang/subdir.am:6:.yin.yin.c:

@ariel-anieli
Copy link
Author

The push force was a rebase.
Any news of the PR?

* replaced suffix rules by pattern rules
* such pattern rules are already used in Makefile.am
* so doing, the suffixes `.lo.bc` & `.o.bc` are no more needed in SUFFIXES
* remove needless macro SUFFIXES; meant for a suffix rule; no more needed
* replaced by a pattern rule in 9b8f605.

Link: https://www.gnu.org/software/make/manual/html_node/Suffix-Rules.html
Link: https://www.gnu.org/s/make/manual/html_node/Pattern-Rules.html
Fixes: 9b8f605 ("build: clean up mgmtd lib protobuf make syntax")
Signed-off-by: Ariel Otilibili <[email protected]>
* replaced suffix rules by pattern rules
* such pattern rules are already used in Makefile.am
* so doing, the suffixes `.lo.bc` & `.o.bc` are no more needed in SUFFIXES.

Link: https://www.gnu.org/software/make/manual/html_node/Suffix-Rules.html
Link: https://www.gnu.org/s/make/manual/html_node/Pattern-Rules.html
Signed-off-by: Ariel Otilibili <[email protected]>
* replaced suffix rule by pattern rule
* same pattern than in 9b8f605
* so doing, SUFFIXES are no more needed.

Link: https://www.gnu.org/software/make/manual/html_node/Suffix-Rules.html
Link: https://www.gnu.org/software/make/manual/html_node/Pattern-Rules.html
Signed-off-by: Ariel Otilibili <[email protected]>
* replaced suffix rules by pattern rules
* `.pb.h`, `.pb.cc`, & `.grpc.pb.cc` are no more needed in SUFFIXES.

Link: https://www.gnu.org/software/make/manual/html_node/Suffix-Rules.html
Link: https://www.gnu.org/s/make/manual/html_node/Pattern-Rules.html
Signed-off-by: Ariel Otilibili <[email protected]>
@frrbot frrbot bot added the bugfix label Dec 14, 2024
@github-actions github-actions bot added size/M and removed size/S labels Dec 14, 2024
@ariel-anieli ariel-anieli changed the title build: improve readability of Makefile.am build, build{lib,yang,grpc}: improve readability of Makefile.am & subdir.am Dec 14, 2024
@ariel-anieli ariel-anieli changed the title build, build{lib,yang,grpc}: improve readability of Makefile.am & subdir.am build, build{lib,yang,grpc}: improve readability of Makefile.am & the respective subdir.am Dec 14, 2024
@ariel-anieli ariel-anieli changed the title build, build{lib,yang,grpc}: improve readability of Makefile.am & the respective subdir.am build: improve readability of Makefile.am & the respective subdir.am Dec 14, 2024
@ariel-anieli
Copy link
Author

Thanks for having taken a look, @donaldsharp; indeed, these extensions do appear only once in the repo. They were introduced for LLVM IR compilation (8fb4037).

# git grep -P '\.l?o\.bc'
Makefile.am:SUFFIXES += .lo.bc .o.bc
Makefile.am:.o.o.bc:
Makefile.am:.lo.lo.bc:

@eqvinox, does the change make sense? Let me know, I will push subsequent PRs. I spotted other places wherein SUFFIXES could be removed: the rules could be more legible.

# git grep -n SUFFIXES
Makefile.am:132:SUFFIXES =
Makefile.am:159:SUFFIXES += .proto .pb-c.c .pb-c.h
Makefile.am:307:SUFFIXES += .lo.bc .o.bc
babeld/Makefile:10:.SUFFIXES:
bfdd/Makefile:10:.SUFFIXES:
bgpd/Makefile:10:.SUFFIXES:
bgpd/rfp-example/librfp/Makefile:10:.SUFFIXES:
bgpd/rfp-example/rfptest/Makefile:10:.SUFFIXES:
doc/Makefile:18:.SUFFIXES:
doc/developer/Makefile:16:.SUFFIXES:
doc/manpages/Makefile:12:.SUFFIXES:
doc/user/Makefile:16:.SUFFIXES:
eigrpd/Makefile:10:.SUFFIXES:
fpm/Makefile:10:.SUFFIXES:
grpc/Makefile:10:.SUFFIXES:
grpc/subdir.am:33:SUFFIXES += .pb.h .pb.cc .grpc.pb.cc
isisd/Makefile:10:.SUFFIXES:
ldpd/Makefile:10:.SUFFIXES:
lib/Makefile:10:.SUFFIXES:
lib/subdir.am:490:SUFFIXES += _clippy.c
lib/subdir.am:503:SUFFIXES += .xref
mgmtd/Makefile:10:.SUFFIXES:
nhrpd/Makefile:10:.SUFFIXES:
ospf6d/Makefile:10:.SUFFIXES:
ospfclient/Makefile:10:.SUFFIXES:
ospfd/Makefile:10:.SUFFIXES:
pathd/Makefile:10:.SUFFIXES:
pbrd/Makefile:10:.SUFFIXES:
pimd/Makefile:10:.SUFFIXES:
qpb/Makefile:10:.SUFFIXES:
ripd/Makefile:10:.SUFFIXES:
ripngd/Makefile:10:.SUFFIXES:
sharpd/Makefile:10:.SUFFIXES:
staticd/Makefile:10:.SUFFIXES:
tests/Makefile:10:.SUFFIXES:
tools/Makefile:10:.SUFFIXES:
vrrpd/Makefile:10:.SUFFIXES:
vtysh/Makefile:10:.SUFFIXES:
watchfrr/Makefile:10:.SUFFIXES:
yang/subdir.am:1:SUFFIXES += .yang .yang.c .yin .yin.c
zebra/Makefile:10:.SUFFIXES:

# git grep -Pn '^\.[a-z_]+([a-z\._])+:' 
Makefile.am:309:.o.o.bc:
Makefile.am:311:.lo.lo.bc:
grpc/subdir.am:42:.proto.pb.cc:
grpc/subdir.am:44:.proto.grpc.pb.cc:
lib/subdir.am:491:.c_clippy.c:
lib/subdir.am:543:.l.c:
lib/subdir.am:545:.y.c:
yang/subdir.am:4:.yang.yang.c:
yang/subdir.am:6:.yin.yin.c:

The push force was a rebase, and the inclusion of the changes I spoke about.
@donaldsharp, @eqvinox, could you have a look? If you need anything from me, let me know.
I am looking forward your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants