Closed Bug 1331663 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: ahal, Assigned: mshal)

References

Details

Attachments

(3 files)

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.
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?
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?
Flags: needinfo?(ahalberstadt)
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.
Flags: needinfo?(ahalberstadt)
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.
mshal, please take this, by all means!
Assignee: nobody → 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.)
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.
Attachment #8830577 - Flags: feedback?(gps)
Attachment #8830577 - Flags: feedback?(ahalberstadt)
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.
Attachment #8830577 - Flags: feedback?(ahalberstadt) → feedback+
> 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.
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 on attachment 8830576 [details]
Bug 1331663 - Allow build backend generation to be invoked without config.status;

https://reviewboard.mozilla.org/r/107322/#review108792
Attachment #8830576 - Flags: review?(gps) → review+
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.
Attachment #8830577 - Flags: feedback?(gps) → feedback+
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?
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.
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bdd9a30f063b
Allow build backend generation to be invoked without config.status; r=gps
https://hg.mozilla.org/mozilla-central/rev/bdd9a30f063b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.