Page MenuHomePhabricator

[Testsuite] Get rid of most of the recursive shared library Makefiles
ClosedPublic

Authored by friss on Oct 6 2019, 6:31 PM.

Details

Summary

I was working on cleaning up our on-device test runs last week, and as
a result I needed to update a lot of test Makefiles in the Swift
tree. I found it especailly annoying to update the build system of
tests that are building mutliple images (typically a shared
library).

The main reason is that you can't see what's (if anything) special
about the build from the main makefile. You always need to open
mutliple Makefiles. Another reason is the way you have to add a
special line to your clean rule to do the right thing, and you never
really know which arguments to pass.

Most of the secondary Makefiles we have are just a couple variable
definitions and then an include of Makefile.rules. This patch removes
most of the secondary Makefiles and replaces them with a direct
invocation of Makefile.rules in the main Makefile. The specificities
of each sub-build are listed right there on the recursive $(MAKE)
call. All the variables that matter are being passed automagically by
make as they have been passed on the command line. The only things you
need to specify are the variables customizating the Makefile.rules
logic for each image.

This patch also deals with the clean rules using this trick to avoid
having to duplicate the recursive invocation in a separate clean rule:

mylib clean::

$(MAKE) -f $(MAKEFILE_RULES) ... $(MAKECMDGOALS)

The MAKECMDGOALS variable at the end contains the list of build goals
that have been passed on the command line. When it's empty, this
invocation just calls the default Makefile.rules rule which is what
you want. When "clean" is passed as a goal, the rule will be invoked
too and it is passed through MAKECMDGOALS invoking the appropriate
cleaning logic from Makefile.rules.

I really like the conciseness of this approach and the fact that
everything is visible in one place. What do others think?

Diff Detail

Event Timeline

friss created this revision.Oct 6 2019, 6:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2019, 6:31 PM

I like the idea of passing the variable values "inline". The MAKECMDGOALS thingy seems... clever. I sort of like it, and it also feels like it might be too much. I suppose that is fine, but.. do we actually need a functional clean rule? I was under the impression that ever since we moved tests to build out-of-tree the clean rule doesn't do much and isn't even being invoked by default. Should we just delete the clean rules altogether? The handful of tests that actually want to clean and rebuild as a part of the test can do something custom...

This is some really tricky Make (wow), but I think the benefit of it being so much shorter and localized outweighs the steeper learning curve. Thanks!

lldb/packages/Python/lldbsuite/test/commands/expression/top-level/Makefile
9

Can you make sure that this is passing what's necessary by removing the default rule from Makefile.rules that initializes CC ?=

lldb/packages/Python/lldbsuite/test/commands/target/create-deps/Makefile
1

= -L$(BUILDDIR) ?
Perhaps := -L. is preferable.

lldb/packages/Python/lldbsuite/test/lang/cpp/namespace_definitions/Makefile
1

Is $(LIBPREFIX) available here? shouldn't this be = instead?

lldb/packages/Python/lldbsuite/test/lang/objc/conflicting-definition/Makefile
1–2

:= and $(BUILDDIR)

2

:=

12

Let's move these past the include Makefile.rules for symmetry.

friss added a comment.Oct 7 2019, 12:26 PM

I like the idea of passing the variable values "inline". The MAKECMDGOALS thingy seems... clever. I sort of like it, and it also feels like it might be too much.

This is why I didn't commit this without review, I do feel the same.

do we actually need a functional clean rule? I was under the impression that ever since we moved tests to build out-of-tree the clean rule doesn't do much and isn't even being invoked by default. Should we just delete the clean rules altogether? The handful of tests that actually want to clean and rebuild as a part of the test can do something custom...

Pretty sure the test runs don't need the clean rule, but it's nice to have when manually iterating over the build phase of a test. OTOH, manually doing a "rm -rf" of the build directory is not a big burden.

Anybody else objects to getting rid of the clean rules?

Pretty sure the test runs don't need the clean rule, but it's nice to have when manually iterating over the build phase of a test. OTOH, manually doing a "rm -rf" of the build directory is not a big burden.

Anybody else objects to getting rid of the clean rules?

I like having the clean rule when I'm debugging a particular test and I'm messing with the Makefile. I don't mind having to manually remove the build directory, but it seems like we could just make that the top-level clean rule. If we go that route, let's put in a safeguard so that you don't accidentally remove your current working directory if you forget to pass the -C flag.

friss updated this revision to Diff 223683.Oct 7 2019, 3:37 PM

Addressed review feedback.

Completely removed the clean code for the Makefiles and reworked it in
Makefile.rules to be much simpler and catch-all.

aprantl added inline comments.Oct 7 2019, 4:03 PM
lldb/packages/Python/lldbsuite/test/make/Makefile.rules
771

I would just error, period. IMHO make clean shouldn'd delete anything that make doesn't know how to rebuild and I don't think we have any rule to restore $(BUILDDIR), do we?

friss marked an inline comment as done.Oct 7 2019, 4:14 PM
friss added inline comments.
lldb/packages/Python/lldbsuite/test/make/Makefile.rules
771

We don't delete $(BUILDDIR), we delete what's inside of it. I would have gotten rid of the rule completely, but having a clean rule is handy in one circumstance: when iterating over the build of a new test (or like I did last week rework a lot of existing test builds). There you want to be able to run the make command by itself, and being able to quickly iterate with make clean all is really nice. I don't think we're taking any risk here as the rule checks whether we are inside a directory that dotest is going to wipe anyway.

aprantl accepted this revision.Oct 7 2019, 4:21 PM

We don't delete $(BUILDDIR), we delete what's inside of it.

Sorry, I missed that detail!

This revision is now accepted and ready to land.Oct 7 2019, 4:21 PM
labath accepted this revision.Oct 8 2019, 2:28 AM
This revision was automatically updated to reflect the committed changes.