1331663 - Refactor build backend generation out of Makefile.in so it can be invoked from cli without a build context

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Andrew Halberstadt [:ahal]]

This is a spin off of:
https://bugzilla.mozilla.org/show_bug.cgi?id=1320194#c51

I'm filing a new bug because this seems like a hard problem in its own right, and there is already a lot happening in bug 1320194. I agree that re-using make logic sounds like a good idea, but after a couple of days trying to implement that suggestion, I feel very out of my depth.

This bug blocks moving python unittests out of make check which impacts the E2E times project.

Comment 1

[Andrew Halberstadt [:ahal]]

Is there any chance a build peer would have time to look into this? I've been trying to learn about makefiles, but still feel very lost trying to figure it out. Or at the very least, could I get a more in depth explanation on what needs to be done to accomplish this?

Comment 2

[Nathan Froyd [:froydnj]]

I read the comment in the original bug, and the code that it was commenting on, and I think I kind of understand what you need to do, but can you try to lay out the problem in prose in this bug?

Comment 3

[Andrew Halberstadt [:ahal]]

Thanks! Actually I was just talking to :mshal on irc and he offered to look into it too, so talk to him before starting anything. But explaining the problem in prose is probably a good idea either way.

The ultimate goal is to be able to run python unittests (and other tests) without requiring any build context. Currently this is not possible because we use test resolving logic baked into the CommonBackend. In bug 1320194, I refactored that logic into a new TestManifestBackend, and then wrote a python function that would emit manifest objects and pass them in directly. Notably, this now happened at test runtime instead of build time.

I also wrote a python function that checks if any of the backend input files were changed and the backend needs to be updated. Understandably, no one liked that I was duplicating make logic in python, but I couldn't do this with any existing make targets because they fail if there is no build context. So this bug is basically about writing a new target to build (or re-build) a backend that can be called both with or without a build context.

Comment 4

[Michael Shal [:mshal]]

froydnj, did you want to tackle this? If so I'll look at other things we need to do to get rid of 'make check'. Otherwise I'm happy to do it.

What gps was suggesting is to essentially move the build-backend regenerating logic out of Makefile.in and into an include-able .mk file so that we can use it in this case where we don't convert the top-level Makefile.in into objdir/Makefile. Then we can remove the test_configuration_changed() function in ahal's patches in bug 1320194.

Comment 5

[Nathan Froyd [:froydnj]]

mshal, please take this, by all means!

Comment 6

[Michael Shal [:mshal]]

I finally got this working, though it's not as clean as I would've hoped. I don't see an easy way to re-use more than just the basic build backend rules, and it requires us to pass in a couple variables. I tried to still have us generate a config.status even for the case where we invoke 'mach python-test' without an objdir, but it ended up being way too complicated. Instead the script to run defaults to config.status and can be overridden by mach when it invokes make.

So while I think this should work, I'd also now be fine with just WONTFIXing this and using ahal's original approach. Although we get to reuse a little code this way, whichever approach we end up with will be a bit hacky.

(Also, does it make sense to use 'hg copy' for a small section of code like this? If so I can redo the patch.)

Comment 7

[Michael Shal [:mshal]]

Attachment: Bug 1331663 - Allow build backend generation to be invoked without config.status;

In bug 1320194 we want to generate a TestManifest backend without first
invoking configure. However, we still need a way to update the backend,
which is normally handled by the top-level Makefile. We can split this
logic out into a separate file, rebuild-backend.mk, which can be invoked
directly as long as the appropriate environment variables are set
(BUILD_BACKEND_FILES, BACKEND_GENERATION_SCRIPT, and PYTHON).

Review commit: https://reviewboard.mozilla.org/r/107322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107322/

Comment 8

[Michael Shal [:mshal]]

Attachment: bug-1320194-followup.patch

Here's how it might look to use this in bug 1320194. ahal, can you confirm this works for you? gps, I figure this might help your review and perhaps deciding whether to go with this or with ahal's dependency-checker-in-python logic.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8830577 [details] [diff] [review]
bug-1320194-followup.patch

Review of attachment 8830577 [details] [diff] [review]:
-----------------------------------------------------------------

Lgtm, though as long as it works I'm pretty impartial to the implementation details on the build side. If we go with this, I'd probably just move the generate_test_metadata() function directly to the gen_test_backend.py script, and instantiate a MozbuildObject there instead of a TestResolver.. But that's orthogonal to this bug.

Comment 10

[Michael Shal [:mshal]]

> If we go with this, I'd probably just move the
> generate_test_metadata() function directly to the gen_test_backend.py
> script, and instantiate a MozbuildObject there instead of a TestResolver..

Good idea, then you could get rid of that annoying getenv() call.

Comment 11

[Andrew Halberstadt [:ahal]]

Attachment: bug 1320194 - Proof of concept v2

I went ahead and implemented my previous suggestion, and figured might as well upload it. It's based on the latest commit series from bug 1320194.

It seems to work great, both with and without an objdir, and solves the gyp ImportError as well.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8830576 [details]
Bug 1331663 - Allow build backend generation to be invoked without config.status;

https://reviewboard.mozilla.org/r/107322/#review108792

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8830577 [details] [diff] [review]
bug-1320194-followup.patch

Review of attachment 8830577 [details] [diff] [review]:
-----------------------------------------------------------------

This is pretty much the solution I had in mind!

I'm, uh, actually surprised the code changes weren't as severe as I was expecting! Nice work.

Comment 14

[Andrew Halberstadt [:ahal]]

I found an edge case where this doesn't work :(..

$ ./mach clobber
$ ./mach python-test python/mozlint  # this generates backend.TestManifest.in
$ ./mach build
$ ./mach mochitest -f plain          # no tests are found because it doesn't think the backend needs to be re-generated

Should we get the build to delete any pre-existing backend.<foo>.in files it finds before generating the new ones?

Comment 15

[Andrew Halberstadt [:ahal]]

Ah, they do get purged in the backend.consume() method.. but the problem is TestManifestBackend never gets 'consumed' as part of the build. This is more of a problem to be solved in bug 1320194, so I'll move discussion over there.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdd9a30f063b
Allow build backend generation to be invoked without config.status; r=gps

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bdd9a30f063b