Move --with-android-ndk, --with-android-toolchain and --with-android-gnu-compiler-version to moz.configure

RESOLVED FIXED in Firefox 48

Status

Firefox Build System
General
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Depends on: 1262019
(Assignee)

Comment 1

2 years ago
Created attachment 8737983 [details]
MozReview Request: Bug 1262020 - Move --target check for mobile/android to moz.configure. r=nalexander

Interestingly, the test was happening too late in old-configure, and you
were more likely to hit other errors first.

Review commit: https://reviewboard.mozilla.org/r/44233/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44233/
Attachment #8737983 - Flags: review?(nalexander)
Attachment #8737984 - Flags: review?(nalexander)
(Assignee)

Comment 2

2 years ago
Created attachment 8737984 [details]
MozReview Request: Bug 1262020 - Move --with-android-ndk, --with-android-toolchain and --with-android-gnu-compiler-version to moz.configure. r=nalexander

Review commit: https://reviewboard.mozilla.org/r/44235/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44235/
Comment on attachment 8737983 [details]
MozReview Request: Bug 1262020 - Move --target check for mobile/android to moz.configure. r=nalexander

https://reviewboard.mozilla.org/r/44233/#review40891

::: mobile/android/moz.configure:13
(Diff revision 1)
> +def check_target(target):
> +    if target.os != 'Android':
> +        log.error('You must specify --target=arm-linux-androideabi (or some '
> +                  'other valid Android target) when building mobile/android.')
> +        die('See https://wiki.mozilla.org/Mobile/Fennec/Android'
> +            '#Setup_Fennec_mozconfig for more information about the necessary '

Drop the fragment (#...).  So just point at the wiki; this documentation is stale enough I don't see it as helping to point people to the specific old doc.
Attachment #8737983 - Flags: review?(nalexander) → review+
Comment on attachment 8737984 [details]
MozReview Request: Bug 1262020 - Move --with-android-ndk, --with-android-toolchain and --with-android-gnu-compiler-version to moz.configure. r=nalexander

https://reviewboard.mozilla.org/r/44235/#review40893

If it's green on try, it's good for me.

::: build/moz.configure/android-ndk.configure:43
(Diff revision 1)
> +        if target.cpu == 'arm' and target.endianness == 'little':
> +            target_format = 'arm-linux-androideabi-%s'
> +        elif target.cpu == 'x86':
> +            target_format = 'x86-%s'
> +        elif target.cpu == 'mips' and target.endianness == 'little':
> +            target_format = 'mipsel-linux-android-%s'

Really?  Live and learn.

::: build/moz.configure/android-ndk.configure:47
(Diff revision 1)
> +        elif target.cpu == 'mips' and target.endianness == 'little':
> +            target_format = 'mipsel-linux-android-%s'
> +        else:
> +            die('Target cpu is not supported.')
> +
> +        toolchain_format = '%s/toolchains/%s/prebuilt/%s-%s'

Seems like you could just have one format string, since `target_format` is always `foo-%s`.

::: build/moz.configure/android-ndk.configure:54
(Diff revision 1)
> +        for version in gnu_compiler_version or ['4.9', '4.8', '4.7']:
> +            target_name = target_format % version
> +            toolchain = toolchain_format % (ndk, target_name,
> +                                            host.kernel.lower(), host.cpu)
> +            log.debug('Trying %s' % quote(toolchain))
> +            if not os.path.isdir(toolchain) and host.cpu == 'x86_64':

I assume these warts are direct translation?  I wonder which is the main path.  Here and below for i686.
Attachment #8737984 - Flags: review?(nalexander) → review+
(Assignee)

Comment 5

2 years ago
(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 8737983 [details]
> MozReview Request: Bug 1262020 - Move --target check for mobile/android to
> moz.configure. r?nalexander
> 
> https://reviewboard.mozilla.org/r/44233/#review40891
> 
> ::: mobile/android/moz.configure:13
> (Diff revision 1)
> > +def check_target(target):
> > +    if target.os != 'Android':
> > +        log.error('You must specify --target=arm-linux-androideabi (or some '
> > +                  'other valid Android target) when building mobile/android.')
> > +        die('See https://wiki.mozilla.org/Mobile/Fennec/Android'
> > +            '#Setup_Fennec_mozconfig for more information about the necessary '
> 
> Drop the fragment (#...).  So just point at the wiki; this documentation is
> stale enough I don't see it as helping to point people to the specific old
> doc.

In fact, the wiki page now points to MDN... I should point there instead, I guess.
(Assignee)

Comment 6

2 years ago
https://reviewboard.mozilla.org/r/44235/#review40893

> I assume these warts are direct translation?  I wonder which is the main path.  Here and below for i686.

What do you mean?
https://reviewboard.mozilla.org/r/44235/#review40893

> What do you mean?

I'd like to get rid of these extra branches.  I see them (now) in `old-configure.in`, but I wonder if they're actually needed, or if we always take one branch or the other.
(Assignee)

Comment 8

2 years ago
https://reviewboard.mozilla.org/r/44235/#review40893

> I'd like to get rid of these extra branches.  I see them (now) in `old-configure.in`, but I wonder if they're actually needed, or if we always take one branch or the other.

They are for two different things. The one you pointed to directly is because some NDKs have compilers that run on x86 and some have compilers that run on x86-64. The ones that have compilers that run on x86 might not be supported anymore, but that's more for me to tell.
The i686 one is because we're using the wrong --target in android mozconfigs, using i386-something instead of i686-something.
(Assignee)

Comment 9

2 years ago
(In reply to Mike Hommey [:glandium] from comment #8)
> but that's more for me to tell.

that's *not* for me to tell
(Assignee)

Comment 10

2 years ago
Fun facts:
- When you push to try with "-p something,all", you get all buildbot builds, and something. But none of the TC builds you'd get for "-p all"
- The android frontend builds build with --disable-compile-environment and --with-android-ndk, which is kind of self-contradictory.
- The android frontend builds's tooltool manifest doesn't install the ndk. Old-configure silently skipped the android ndk check, but moz.configure doesn't, so it fails.
(Assignee)

Comment 11

2 years ago
Comment on attachment 8737983 [details]
MozReview Request: Bug 1262020 - Move --target check for mobile/android to moz.configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44233/diff/1-2/
Attachment #8737983 - Attachment description: MozReview Request: Bug 1262020 - Move --target check for mobile/android to moz.configure. r?nalexander → MozReview Request: Bug 1262020 - Move --target check for mobile/android to moz.configure. r=nalexander
Attachment #8737984 - Attachment description: MozReview Request: Bug 1262020 - Move --with-android-ndk, --with-android-toolchain and --with-android-gnu-compiler-version to moz.configure. r?nalexander → MozReview Request: Bug 1262020 - Move --with-android-ndk, --with-android-toolchain and --with-android-gnu-compiler-version to moz.configure. r=nalexander
(Assignee)

Comment 12

2 years ago
Comment on attachment 8737984 [details]
MozReview Request: Bug 1262020 - Move --with-android-ndk, --with-android-toolchain and --with-android-gnu-compiler-version to moz.configure. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44235/diff/1-2/
(Assignee)

Updated

2 years ago
Attachment #8737984 - Flags: review+ → review?(nalexander)
Attachment #8737984 - Flags: review?(nalexander) → review+
Comment on attachment 8737984 [details]
MozReview Request: Bug 1262020 - Move --with-android-ndk, --with-android-toolchain and --with-android-gnu-compiler-version to moz.configure. r=nalexander

https://reviewboard.mozilla.org/r/44235/#review41043

I'm fine with this approach.

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/20c9113f6bee
https://hg.mozilla.org/mozilla-central/rev/26b2eb531d94
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1262890

Comment 16

2 years ago
Since this patch landed, I need an explicit TOOLCHAIN_PREFIX in my mingw mozconfig for cross compilation (like TOOLCHAIN_PREFIX=i686-w64-mingw32-). It wasn't required before the commit and the usual way is that configure scrtipt to finds it based on --target=... (well, or --host=...).

Is it intentional? Can we restore previous behaviour (I may write a patch, I just need to know if it's wanted)?
(Assignee)

Comment 17

2 years ago
(In reply to Jacek Caban from comment #16)
> Since this patch landed, I need an explicit TOOLCHAIN_PREFIX in my mingw
> mozconfig for cross compilation (like TOOLCHAIN_PREFIX=i686-w64-mingw32-).
> It wasn't required before the commit and the usual way is that configure
> scrtipt to finds it based on --target=... (well, or --host=...).
> 
> Is it intentional? Can we restore previous behaviour (I may write a patch, I
> just need to know if it's wanted)?

Please file a new bug.

Updated

2 years ago
Depends on: 1263876

Updated

4 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.