aleth [:aleth] Assignee
	Description • 8 years ago

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

	aleth [:aleth] Assignee
	Comment 2 • 8 years ago

m-c try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=299f801012f471668d92e4400cf2e308044dcbab

	Ralph Giles (:rillian)
	Comment 3 • 8 years ago

Assuming we're allowed conditionals in mozconfig, this may also fix some of the spidermonkey jobs we had trouble with.

	Gregory Szorc [:gps]
	Comment 4 • 8 years ago

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.

	Ralph Giles (:rillian)
	Comment 5 • 8 years ago

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

	Gregory Szorc [:gps]
	Comment 6 • 8 years ago

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.

	aleth [:aleth] Assignee
	Comment 7 • 8 years ago

(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?

	Ralph Giles (:rillian)
	Comment 8 • 8 years ago

(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.

	aleth [:aleth] Assignee
	Comment 9 • 8 years ago

(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.

	aleth [:aleth] Assignee
	Comment 10 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/970701ece1a39f49648632c62b90f65da742db5c
Bug 1324120 - Ensure mozconfig.rust uses the TOOLTOOL_DIR when available. r=gps

	aleth [:aleth] Assignee
	Comment 11 • 8 years ago

Not sure if backouts require review?

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/970701ece1a3

	aleth [:aleth] Assignee
	Comment 13 • 8 years ago

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

	Gregory Szorc [:gps]
	Comment 14 • 8 years ago

Looks like the needinfo against me isn't needed any more.

	aleth [:aleth] Assignee
	Comment 15 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 8 years ago
bugherder

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