1266368 - Move rust.m4 to moz.configure

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

This should be fairly straightforward now.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: bug 1266368 - move rust.m4 to configure.

Review commit: https://reviewboard.mozilla.org/r/52982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52982/

Comment 2

[(not currently active) Ted Mielczarek]

There are a few remaining checks in old-configure that I didn't move as part of this patch. I started working on a follow-up patch to move them.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8753069 [details]
bug 1266368 - move rust.m4 to configure.

https://reviewboard.mozilla.org/r/52982/#review49928

::: build/moz.configure/rust.configure:7
(Diff revision 1)
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +rustc = check_prog('RUSTC', ['rustc'], allow_missing=True)

I know this is transposing the current test from old.configure, but we might as well do things in a nicer way.

The check should be skipped when rust support is not enabled. This can be done by providing a @depends function instead of ['rustc'] and having that function return ['rustc'] when the test should happen, and None when not.

Also,in python configure land, we can do things in a nicer way: define the option as opt-out (option('--disable-rust')), check that whether the compiler is installed if rust is enabled, and error out when it's missing *only* when rust was *explicitly* enabled (value.origin != 'default').

::: build/moz.configure/rust.configure:29
(Diff revision 1)
> +@depends('--enable-rust', rustc, rustc_version)
> +@imports(_from='textwrap', _import='dedent')
> +def rust_compiler(value, rustc, rustc_version):
> +    if value:
> +        if not rustc:
> +            die(dedent('''Rust compiler not found.

dedent removes the indentation level to match that of the first line. This means it won't dedent in your case.
The usual way to use it is to do:
    dedent('''\
        foo
        bar
    ''')

The first backslash being there to have no newline before the first actual line of text.

::: build/moz.configure/rust.configure:32
(Diff revision 1)
> +    if value:
> +        if not rustc:
> +            die(dedent('''Rust compiler not found.
> +            To compile rust language sources, you must have 'rustc' in your path.
> +            See http://www.rust-lang.org/ for more information.'''))
> +        if rustc_version < '1.5.0':

< '1.5' should work too.

::: build/moz.configure/rust.configure:40
(Diff revision 1)
> +    else:
> +        return False

Probably better off not returning a value at all (equivalent to return None), which will leave the variable not set at all.

::: build/moz.configure/rust.configure:61
(Diff revision 1)
> +        # The canonical list of targets supported can be derived from:
> +        #
> +        # https://github.com/rust-lang/rust/tree/master/mk/cfg

I wish they allowed to not have to care about the machine type, which clang is perfectly happy with skipping.
https://github.com/rust-lang/rust/issues/33147

::: build/moz.configure/rust.configure:68
(Diff revision 1)
> +        rustc_target = {
> +            # DragonFly
> +            ('x86_64', 'Dragonfly'): 'x86_64-unknown-dragonfly',
> +            # FreeBSD, GNU/kFreeBSD
> +            ('x86', 'FreeBSD'): 'i686-unknown-freebsd',
> +            ('x86_64', 'FreeBSD'): 'x86_64-unknown-freebsd',
> +            # NetBSD
> +            ('x86_64', 'NetBSD'): 'x86_64-unknown-netbsd',
> +            # OpenBSD
> +            ('x86_64', 'OpenBSD'): 'x86_64-unknown-openbsd',
> +            # Linux
> +            ('x86', 'Linux'): 'i686-unknown-linux-gnu',
> +            # Linux
> +            ('x86_64', 'Linux'): 'x86_64-unknown-linux-gnu',
> +            # OS X and iOS
> +            ('x86', 'OSX'): 'i686-apple-darwin',
> +            ('x86', 'iOS'): 'i386-apple-ios',
> +            ('x86_64', 'OSX'): 'x86_64-apple-darwin',
> +            # Android
> +            ('x86', 'Android'): 'i686-linux-android',
> +            ('arm', 'Android'): 'arm-linux-androideabi',
> +            # Windows
> +            # XXX better detection of CXX needed here, to figure out whether
> +            # we need i686-pc-windows-gnu instead, since mingw32 builds work.
> +            ('x86', 'WINNT'): 'i586-pc-windows-msvc',
> +            ('x86_64', 'WINNT'): 'x86_64-pc-windows-msvc',
> +        }.get((target.cpu, os_or_kernel), None)

I wonder if we shouldn't more broadly derive from --target here (from target.raw_cpu and target.raw_os).

::: build/moz.configure/rust.configure:123
(Diff revision 1)
> +            log.debug('Executing: `%s`', quote(*cmd))
> +            proc = subprocess.Popen(cmd, stdout=subprocess.PIPE,
> +                                    stderr=subprocess.PIPE)
> +            stdout, stderr = proc.communicate()
> +            retcode = proc.wait()
> +            if retcode != 0:
> +                log.debug('The command returned non-zero exit status %d.', retcode)
> +                for out, desc in ((stdout, 'output'), (stderr, 'error output')):
> +                    if out:
> +                        log.debug('Its %s was:', desc)
> +                        with LineIO(lambda l: log.debug('| %s', l)) as o:
> +                            o.write(out)
> +            if retcode != 0 or not (os.path.exists(out_path) and os.path.getsize(out_path) > 0):
> +                die('Cannot compile for {} with {}'.format(target.alias, rustc))

bug 1269513 has a nice wrapper function for this.

Comment 4

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/52982/#review49928

> I know this is transposing the current test from old.configure, but we might as well do things in a nicer way.
> 
> The check should be skipped when rust support is not enabled. This can be done by providing a @depends function instead of ['rustc'] and having that function return ['rustc'] when the test should happen, and None when not.
> 
> Also,in python configure land, we can do things in a nicer way: define the option as opt-out (option('--disable-rust')), check that whether the compiler is installed if rust is enabled, and error out when it's missing *only* when rust was *explicitly* enabled (value.origin != 'default').

Doesn't that change the behavior by making it opt-out instead of opt-in? That doesn't seem like what we want.

> I wonder if we shouldn't more broadly derive from --target here (from target.raw_cpu and target.raw_os).

Let's move that to a followup, I'd rather not change the behavior much in this patch. Filed bug 1273456.

> bug 1269513 has a nice wrapper function for this.

Thanks, I was thinking about doing some refactoring as I was copy/pasting so much of this function.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> https://reviewboard.mozilla.org/r/52982/#review49928
> 
> > I know this is transposing the current test from old.configure, but we might as well do things in a nicer way.
> > 
> > The check should be skipped when rust support is not enabled. This can be done by providing a @depends function instead of ['rustc'] and having that function return ['rustc'] when the test should happen, and None when not.
> > 
> > Also,in python configure land, we can do things in a nicer way: define the option as opt-out (option('--disable-rust')), check that whether the compiler is installed if rust is enabled, and error out when it's missing *only* when rust was *explicitly* enabled (value.origin != 'default').
> 
> Doesn't that change the behavior by making it opt-out instead of opt-in?
> That doesn't seem like what we want.

Damn, I thought it was opt-out, but it looks like it isn't... I guess I was confused by the rustc check always happening...

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8753069 [details]
bug 1266368 - move rust.m4 to configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52982/diff/1-2/

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8753069 [details]
bug 1266368 - move rust.m4 to configure.

https://reviewboard.mozilla.org/r/52982/#review56276

::: build/moz.configure/rust.configure:10
(Diff revisions 1 - 2)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -rustc = check_prog('RUSTC', ['rustc'], allow_missing=True)
> +option('--enable-rust', help='Include Rust language sources')
> +
> +@depends('--enable-rust')
> +def rust_prog_names(value):

rust_compiler?

::: build/moz.configure/rust.configure:13
(Diff revisions 1 - 2)
> +    else:
> +        return None

You can skip this. Not returning does the same.

::: build/moz.configure/rust.configure:134
(Diff revisions 1 - 2)
> -                        with LineIO(lambda l: log.debug('| %s', l)) as o:
> -                            o.write(out)
> -            if retcode != 0 or not (os.path.exists(out_path) and os.path.getsize(out_path) > 0):
>                  die('Cannot compile for {} with {}'.format(target.alias, rustc))
> +            check_cmd_output(*cmd, onerror=failed)
> +            if not (os.path.exists(out_path) and os.path.getsize(out_path) > 0):

not os.path.exists(...) or os.path.getsize(...) == 0

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8753069 [details]
bug 1266368 - move rust.m4 to configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52982/diff/2-3/

Comment 9

[(not currently active) Ted Mielczarek]

My try push failed in a few places, notably:
1) Windows builds set `RUSTC="$topsrcdir/rustc/bin/rustc"`:
https://dxr.mozilla.org/mozilla-central/rev/ddb6b30149221f00eb5dd180530e9033093d4c2b/build/mozconfig.rust#6

which apparently works in old-configure but not in moz.configure:
 02:51:44     INFO -  checking for rustc... not found
 02:51:44     INFO -  DEBUG: rustc: Trying c:/builds/moz2_slave/try-w32-0000000000000000000000/build/src/rustc/bin/rustc
02:51:44 INFO - ERROR: Cannot find rustc

...due to the missing .exe extension. We should fix that.

2) This breaks artifact builds because `--enable-rust` is now in toolchain.configure, which doesn't get evaluated for `--disable-compile-environment` builds:
https://treeherder.mozilla.org/logviewer.html#?job_id=22433354&repo=try#L1370

I guess we could just move the option bit up a level?

Comment 10

[(not currently active) Ted Mielczarek]

In person, glandium says we should just make the artifact builds stop including that in their mozconfig.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 8753069 [details]
bug 1266368 - move rust.m4 to configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52982/diff/3-4/

Comment 12

[(not currently active) Ted Mielczarek]

I re-pushed to try on top of the patch from bug 1280220 which should fix the Windows issue.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 8753069 [details]
bug 1266368 - move rust.m4 to configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52982/diff/4-5/

Comment 14

[(not currently active) Ted Mielczarek]

...and again because I screwed up the tests for that patch, but I re-pushed with the patch updated to fix review comments anyway.

Comment 15

[(not currently active) Ted Mielczarek]

Ugh, this is still broken on Windows:
 05:13:57 INFO - WindowsError: [Error 32] The process cannot access the file because it is being used by another process: u'c:\\users\\cltbld\\appdata\\local\\temp\\conftesthkeu7m.rlib'

Comment 16

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec9043d72483

Comment 17

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eceea12f43fb

Comment 18

[(not currently active) Ted Mielczarek]

The *build* succeeded on Try, but some of the configure unit tests failed because I changed find_program and apparently didn't change the VFS test impl.

Comment 19

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a5a4e54d2bf

Comment 20

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b555450a4d80e208e33342620d27294ee900d59
bug 1266368 - move rust.m4 to configure. r=glandium

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b555450a4d8

Comment 22

[Jan Beich]

https://reviewboard.mozilla.org/r/52982/#review59984

::: build/moz.configure/rust.configure:79
(Diff revision 5)
> +        rustc_target = {
> +            # DragonFly
> +            ('x86_64', 'Dragonfly'): 'x86_64-unknown-dragonfly',
> +            # FreeBSD, GNU/kFreeBSD
> +            ('x86', 'FreeBSD'): 'i686-unknown-freebsd',
> +            ('x86_64', 'FreeBSD'): 'x86_64-unknown-freebsd',

FreeBSD needs to test target.kernel like Linux in order to include GNU/kFreeBSD. glandium, can you confirm?

Comment 23

[Mike Hommey [:glandium]]

presumably, kFreeBSD has/would have a different triplet.