Closed Bug 1324120 Opened 3 years ago Closed 3 years ago

Ensure mozconfig.rust uses the TOOLTOOL_DIR when available

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(2 files)

The now default-enabled rust fails configure on c-c builds because mozconfig.rust assumes topsrcdir is the same as the TOOLTOOL_DIR.

We can do the same trick here that is used in mozconfig.gtk, to ensure TOOLTOOL_DIR is used directly when it is defined.

This might also make the following commit for hazard builds obsolete, assuming mozconfig.rust is parsed for those as well: https://hg.mozilla.org/mozilla-central/rev/a740323ace17
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Assuming we're allowed conditionals in mozconfig, this may also fix some of the spidermonkey jobs we had trouble with.
Depends on: 1324607
Comment on attachment 8819442 [details] [diff] [review]
Ensure mozconfig.rust uses the TOOLTOOL_DIR when available

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

My only concern about this is that ${FOO:-default} syntax is not supported by all shells. But it is supported by the bourne shell (we execute mozconfigs with /bin/sh), so I /think/ we should be OK.

I'm not a shell expert, so I wouldn't be surprised if I'm wrong. You may want to rewrite this as `if [ -n "${TOOLTOOL_DIR}" ] ...` just in case.
Attachment #8819442 - Flags: review?(gps) → review+
FWIW, this syntax is on apenwarr's list of effectively-portable shell techniques[1]. It's nicer to read, so I think it's worth trying in tree. We can go back to an explicit `if` should people have trouble in practice.

It's also easier to support in a hypothetical declarative-only non-shell mozconfig parser, which I think is what inspired my worry in comment #3.

[1] http://apenwarr.ca/log/?m=201102#28
Unfortunately we can't easily transition to a declarative only non-shell mozconfig parser because a lot of people in the wild have mozconfigs that leverage the fact that they are fully-featured shell scripts. The best we can do is provide an alternate to mozconfigs and gradually phase out mozconfigs in their current form. It's a bit low on the priority list because there doesn't seem to be a significant gain from doing that.
(In reply to Gregory Szorc [:gps] from comment #4)
> My only concern about this is that ${FOO:-default} syntax is not supported
> by all shells. But it is supported by the bourne shell (we execute
> mozconfigs with /bin/sh), so I /think/ we should be OK.
> 
> I'm not a shell expert, so I wouldn't be surprised if I'm wrong. You may
> want to rewrite this as `if [ -n "${TOOLTOOL_DIR}" ] ...` just in case.

We have this syntax in-tree already (albeit for Linux only), so I think we should be OK:
http://searchfox.org/mozilla-central/source/build/unix/mozconfig.gtk#5(In reply to aleth [:aleth] from comment #0)

> This might also make the following commit for hazard builds obsolete,
> assuming mozconfig.rust is parsed for those as well:
> https://hg.mozilla.org/mozilla-central/rev/a740323ace17

Do you think this can be backed out after this patch lands?
Flags: needinfo?(gps)
(In reply to aleth [:aleth] from comment #7)

> > https://hg.mozilla.org/mozilla-central/rev/a740323ace17
> 
> Do you think this can be backed out after this patch lands?

I think so. This patch should address the same issue.
Keywords: checkin-needed
(In reply to aleth [:aleth] from comment #7)
> > I'm not a shell expert, so I wouldn't be surprised if I'm wrong. You may
> > want to rewrite this as `if [ -n "${TOOLTOOL_DIR}" ] ...` just in case.
> 
> We have this syntax in-tree already (albeit for Linux only), so I think we
> should be OK:
> http://searchfox.org/mozilla-central/source/build/unix/mozconfig.gtk#5(In
> reply to aleth [:aleth] from comment #0)

Let's try it and follow up with a change if required.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/970701ece1a39f49648632c62b90f65da742db5c
Bug 1324120 - Ensure mozconfig.rust uses the TOOLTOOL_DIR when available. r=gps
Not sure if backouts require review?
Attachment #8821104 - Flags: review?(gps)
Keywords: leave-open
https://hg.mozilla.org/comm-central/rev/8dbc018bf883191d7e851417d6e664483fc7d90c
Bug 1324120 - Ensure mozconfig.rust uses the TOOLTOOL_DIR when available, c-c synced file. r=gps
Looks like the needinfo against me isn't needed any more.
Flags: needinfo?(gps)
Attachment #8821104 - Flags: review?(gps) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d1dbc6426dd1c8b89488f62ae859a3c8a7aa0c
Bug 1324120 - Back out changeset a740323ace17 as mozconfig.rust now already uses the TOOLTOOL_DIR. r=gps
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/a9d1dbc6426d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.