Closed Bug 1410475 Opened 2 years ago Closed 2 years ago

Add support for detecting whether c-c is topdir in mozconfigs.

Categories

(Thunderbird :: Build Config, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 58.0

People

(Reporter: tomprince, Assigned: tomprince)

Details

Attachments

(2 files)

We are transitioning comm-central (in particular, Thunderbird) from building as a parent directory of mozilla-central to as a subdirectory of it.

In order to have mozconfig files that support building in either configuration during the transition, without duplicating the logic in every mozconfig file, there needs to exist a file that exists at the same path in mozilla-central and comm-central.
Comment on attachment 8920656 [details]
Bug 1410475: Add support for detecting whether c-c is topdir in mozconfigs;

https://reviewboard.mozilla.org/r/191630/#review196838

::: mail/config/mozconfigs/linux64/nightly:1
(Diff revision 1)
> +. "$topsrcdir/build/mozconfig.comm-support"

I've only changed this one mozconfig since it is the only one I've got setup to test building with m-c as  topdir. I'll follow this up with patches that refactor our mozconfigs.
Attachment #8920655 - Flags: review?(core-build-config-reviews)
Attachment #8920656 - Flags: review?(core-build-config-reviews)
Comment on attachment 8920655 [details]
Bug 1410475: Add support for detecting whether c-c is topdir in mozconfigs;

https://reviewboard.mozilla.org/r/191636/#review197828

::: build/mozconfig.comm-support:22
(Diff revision 1)
> +# mozconfig.
> +
> +
> +# Note that the top-level mozconfig file is in $2.
> +
> +if [ "$(dirname "$2")" == "$topsrcdir" ]; then

The extra quoting here around "$2" is odd - just "$(dirname $2)" should suffice.

::: build/mozconfig.comm-support:22
(Diff revision 1)
> +# mozconfig.
> +
> +
> +# Note that the top-level mozconfig file is in $2.
> +
> +if [ "$(dirname "$2")" == "$topsrcdir" ]; then

We should probably use = instead of == since the latter is a bash extension.

::: build/mozconfig.comm-support:36
(Diff revision 1)
> +    . "$topsrcdir/comm/build/mozconfig.comm-support"
> +  else
> +    echo "ERROR: Unknown build directory layout."
> +    exit 1
> +  fi
> +elif [ "$(dirname "$2")" == "$(dirname "$topsrcdir")" ]; then

Similarly here, just have a single set of quotes like "$(dirname $2)" and "$(dirname $topsrcdir)"

::: build/mozconfig.comm-support:36
(Diff revision 1)
> +    . "$topsrcdir/comm/build/mozconfig.comm-support"
> +  else
> +    echo "ERROR: Unknown build directory layout."
> +    exit 1
> +  fi
> +elif [ "$(dirname "$2")" == "$(dirname "$topsrcdir")" ]; then

And replace == with = here.
Attachment #8920655 - Flags: review+
Comment on attachment 8920656 [details]
Bug 1410475: Add support for detecting whether c-c is topdir in mozconfigs;

https://reviewboard.mozilla.org/r/191632/#review197836
Attachment #8920656 - Flags: review+
Attachment #8920655 - Flags: review?(core-build-config-reviews)
Attachment #8920656 - Flags: review?(core-build-config-reviews)
Comment on attachment 8920655 [details]
Bug 1410475: Add support for detecting whether c-c is topdir in mozconfigs;

https://reviewboard.mozilla.org/r/191636/#review197828

> The extra quoting here around "$2" is odd - just "$(dirname $2)" should suffice.

I put the extra quotes in because [shellcheck](https://github.com/koalaman/shellcheck) was complaining about it. It could be (but hopefully usually isn't) that `$2` has spaces in it, in which case the quoting is correct. I imagine lots of other stuff might break, though.
(In reply to Tom Prince [:tomprince] from comment #7)
> > The extra quoting here around "$2" is odd - just "$(dirname $2)" should suffice.
> 
> I put the extra quotes in because
> [shellcheck](https://github.com/koalaman/shellcheck) was complaining about
> it. It could be (but hopefully usually isn't) that `$2` has spaces in it, in
> which case the quoting is correct. I imagine lots of other stuff might
> break, though.

Interesting, so it does. Feel free to keep it as you had it then!
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/integration/autoland/rev/c7ae1c6f2a49
Add support for detecting whether c-c is topdir in mozconfigs; r=mshal
https://hg.mozilla.org/mozilla-central/rev/c7ae1c6f2a49
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 58.0
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/comm-central/rev/539567e1f723
Add support for detecting whether c-c is topdir in mozconfigs; r=mshal
Tom, just out of interest, why are there changes in mail/config/mozconfigs/linux64/nightly and no other config?
Flags: needinfo?(mozilla)
> I've only changed this one mozconfig since it is the only one I've got setup to test building with m-c as  topdir. I'll follow this up with patches that refactor our mozconfigs.
Flags: needinfo?(mozilla)
Comment on attachment 8920655 [details]
Bug 1410475: Add support for detecting whether c-c is topdir in mozconfigs;

Looks good to me, thanks for working on it!
Attachment #8920655 - Flags: review?(philipp) → review+
Attachment #8920656 - Flags: review?(philipp) → review+
Relanded most of the patch, apart from the changes to mail/config/mozconfigs/linux64/nightly:
https://hg.mozilla.org/releases/comm-beta/rev/c43f5499c52482498c955966200c02507e0b50b1
You need to log in before you can comment on or make changes to this bug.