Closed Bug 1584567 Opened 2 months ago Closed Last month

Remove support for conditional names

Categories

(Firefox Build System :: Mach Core, task)

task
Not set

Tracking

(firefox71 fixed)

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(2 files)

From https://bugzilla.mozilla.org/show_bug.cgi?id=1521996#c18:

I know I reviewed this and should have thought of this back then, but I was doing some debugging in bug 1580280 and realized this causes us to invoke configure before mach even parses the command line. So e.g, every mach invocation ever is going into configure.

So I just wanted to point out that mach supports a callable parser, so it would be totally possible to have different command line arguments for Android vs Desktop in a single mach implementation (as well as branching in the implementation itself). In fact, mochitest's ArgumentParser has this exact condition already:
https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/testing/mochitest/mach_commands.py#212

In other words, we could scrap conditional_names entirely and re-work the install and run commands to have different arguments and branches depending on whether we have an Android build or not. Wish I thought of this during review.

Anyway, it's not a super high priority or anything.. but I think it would be good to avoid using more conditional_names in the future.

So turns out I was wrong. This doesn't cause us to invoke configure and it has nothing to do with bug 1580280.

But before I realized this, I went ahead and implemented the patches to fix it. I'd still prefer to get them landed as I think conditional_names can be problematic (breaks assumptions by changing the names of commands at runtime). Mach is simpler and cleaner without them.

However, these patches aren't solving any current problems. So if they cause problems or pose risk I'm happy to abandon them.

Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b50c4cdef2ac
Merge desktop + android commands (run and install) back together, r=nalexander
https://hg.mozilla.org/integration/autoland/rev/4fbb6d144880
[mach] Remove support for conditional_names, r=nalexander
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Regressions: 1585828
You need to log in before you can comment on or make changes to this bug.