Closed Bug 1262020 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

No description provided.
Depends on: 1262019
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)
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+
(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.
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.
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.
(In reply to Mike Hommey [:glandium] from comment #8)
> but that's more for me to tell.

that's *not* for me to tell
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.
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
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/
Attachment #8737984 - Flags: review+ → review?(nalexander)
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.
https://hg.mozilla.org/mozilla-central/rev/20c9113f6bee
https://hg.mozilla.org/mozilla-central/rev/26b2eb531d94
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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)?
(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.
Depends on: 1263876
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.