1259382 - Move CC/CXX/etc. to moz.configure

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1259382 - Make mozbuild.shellutil.quote more useful for e.g. creating printable command lines. r?ted

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

Comment 2

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1259382 - Remove support for Intel C/C++ compiler. r?ted

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

Comment 3

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1259382 - Make add_old_configure_assignment work like set_config, accepting both @depends functions and literal values for both arguments it takes. r?ted

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

Comment 4

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1259382 - Get a full path to the compiler wrapper. r?ted

In the near future, we are going to try running the compiler wrapper
from python code, and that will work better if we have the full path
to it.

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

Comment 5

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1259382 - Move CC/CXX/HOST_CC/HOST_CXX to moz.configure. r?ted

At the same time, we improve things slightly by deriving HOST_CC from CC
in a smarter way, as well as CXX from CC, which we weren't doing
previously.

Many related things are not moved at the same time to keep the patch
somehow "small".

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

Comment 6

[Mike Hommey [:glandium]]

This is only a third of my current queue. If things look incomplete (like, if you wonder why the version check isn't moved as well), it's because I'm splitting things in a way that hopefully will help review. At least it should be easier to review than a big patch adding 1000 lines.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8738109 [details]
MozReview Request: Bug 1259382 - Make mozbuild.shellutil.quote more useful for e.g. creating printable command lines. r?ted

https://reviewboard.mozilla.org/r/44323/#review42301

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8738110 [details]
MozReview Request: Bug 1259382 - Remove support for Intel C/C++ compiler. r?ted

https://reviewboard.mozilla.org/r/44325/#review42303

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8738111 [details]
MozReview Request: Bug 1259382 - Make add_old_configure_assignment work like set_config, accepting both @depends functions and literal values for both arguments it takes. r?ted

https://reviewboard.mozilla.org/r/44327/#review42305

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8738112 [details]
MozReview Request: Bug 1259382 - Get a full path to the compiler wrapper. r?ted

https://reviewboard.mozilla.org/r/44329/#review42307

Comment 11

[Mike Hommey [:glandium]]

I'm going to land the first three that are not 100% related, and one of which I'm relying upon in a patch I'm going to attach in another bug.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a842f5b1123
https://hg.mozilla.org/integration/mozilla-inbound/rev/22b9ab9b197b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c370d840756

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4a842f5b1123
https://hg.mozilla.org/mozilla-central/rev/22b9ab9b197b
https://hg.mozilla.org/mozilla-central/rev/8c370d840756

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 8738113 [details]
MozReview Request: Bug 1259382 - Move CC/CXX/HOST_CC/HOST_CXX to moz.configure. r?ted

https://reviewboard.mozilla.org/r/44331/#review42347

Just a few questions and suggestions. The meat of this is tricky to read, but I understand what it's doing, at least.

::: browser/config/mozconfigs/linux64/artifact:10
(Diff revision 1)
>  
>  . "$topsrcdir/browser/config/mozconfigs/linux64/nightly"
>  
>  ac_add_options --enable-artifact-builds
> +unset CC
> +unset CXX

Why do we need this?

::: browser/config/mozconfigs/linux64/artifact:10
(Diff revision 1)
>  
>  . "$topsrcdir/browser/config/mozconfigs/linux64/nightly"
>  
>  ac_add_options --enable-artifact-builds
> +unset CC
> +unset CXX

Why do we need this?

::: build/autoconf/toolchain.m4:124
(Diff revision 1)
> -AC_CHECK_PROGS(CC, "${TOOLCHAIN_PREFIX}gcc", :)
> -unset ac_cv_prog_CC
>  AC_PROG_CC
> -AC_CHECK_PROGS(CXX, "${TOOLCHAIN_PREFIX}g++", :)
> -unset ac_cv_prog_CXX
>  AC_PROG_CXX

Are these two lines still doing something useful?

::: build/moz.configure/toolchain.configure:177
(Diff revision 1)
> +    suffix = {
> +        'C': '.c',
> +        'C++': '.cpp',
> +    }[language]
> +
> +    fd, path = mkstemp(prefix='conftest.', suffix=suffix)

You could also use NamedTemporaryFile(remove=False) here, which also works as a context manager.

::: build/moz.configure/toolchain.configure:193
(Diff revision 1)
> +        cmd = compiler + ['-E', path]
> +        log.debug('Executing: `%s`', quote(*cmd))
> +        proc = subprocess.Popen(cmd, stdout=subprocess.PIPE,
> +                                stderr=subprocess.PIPE)
> +        stdout, stderr = proc.communicate()
> +        retcode = proc.wait()

subprocess.check_output might make this a little simpler.

::: build/moz.configure/toolchain.configure:226
(Diff revision 1)
> +    @depends(host_or_target, host, toolchain_prefix)
> +    def default_c_compilers(host_or_target, host, toolchain_prefix):
> +        if host_or_target.os == 'WINNT':
> +            return ('cl', 'clang-cl', 'gcc', 'clang')
> +        if host_or_target.os == 'Darwin':
> +            return ('clang',)

Do we not support gcc on OS X at all now, or is it just that you have to explicitly set CC=gcc?

Comment 15

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> Comment on attachment 8738113 [details]
> MozReview Request: Bug 1259382 - Move CC/CXX/HOST_CC/HOST_CXX to
> moz.configure. r?ted
> 
> https://reviewboard.mozilla.org/r/44331/#review42347
> 
> Just a few questions and suggestions. The meat of this is tricky to read,
> but I understand what it's doing, at least.
> 
> ::: browser/config/mozconfigs/linux64/artifact:10
> (Diff revision 1)
> >  
> >  . "$topsrcdir/browser/config/mozconfigs/linux64/nightly"
> >  
> >  ac_add_options --enable-artifact-builds
> > +unset CC
> > +unset CXX
> 
> Why do we need this?

Because artifact builds don't include toolchain.configure, so CC is not a defined option, but mozconfig injects it, and configure complains. Eventually, the mozconfigs should be arranged in the opposite direction (adding CC to the mozconfigs that do compile)

> ::: build/autoconf/toolchain.m4:124
> (Diff revision 1)
> > -AC_CHECK_PROGS(CC, "${TOOLCHAIN_PREFIX}gcc", :)
> > -unset ac_cv_prog_CC
> >  AC_PROG_CC
> > -AC_CHECK_PROGS(CXX, "${TOOLCHAIN_PREFIX}g++", :)
> > -unset ac_cv_prog_CXX
> >  AC_PROG_CXX
> 
> Are these two lines still doing something useful?

Unfortunately yes. At least for now.

> ::: build/moz.configure/toolchain.configure:177
> (Diff revision 1)
> > +    suffix = {
> > +        'C': '.c',
> > +        'C++': '.cpp',
> > +    }[language]
> > +
> > +    fd, path = mkstemp(prefix='conftest.', suffix=suffix)
> 
> You could also use NamedTemporaryFile(remove=False) here, which also works
> as a context manager.

Doesn't NamedTemporaryFile leave the file open, which would prevent the subprocess from reading it on windows?

> ::: build/moz.configure/toolchain.configure:193
> (Diff revision 1)
> > +        cmd = compiler + ['-E', path]
> > +        log.debug('Executing: `%s`', quote(*cmd))
> > +        proc = subprocess.Popen(cmd, stdout=subprocess.PIPE,
> > +                                stderr=subprocess.PIPE)
> > +        stdout, stderr = proc.communicate()
> > +        retcode = proc.wait()
> 
> subprocess.check_output might make this a little simpler.

Not when you want stdout and stderr separated.

> ::: build/moz.configure/toolchain.configure:226
> (Diff revision 1)
> > +    @depends(host_or_target, host, toolchain_prefix)
> > +    def default_c_compilers(host_or_target, host, toolchain_prefix):
> > +        if host_or_target.os == 'WINNT':
> > +            return ('cl', 'clang-cl', 'gcc', 'clang')
> > +        if host_or_target.os == 'Darwin':
> > +            return ('clang',)
> 
> Do we not support gcc on OS X at all now, or is it just that you have to
> explicitly set CC=gcc?

We have this in build/autoconf/compiler-opts.m4:

      echo gcc is known to be broken on OS X, please use clang.
      echo see http://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions/Mac_OS_X_Prerequisites
      echo for more information.
      exit 1

Comment 16

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #15)
> > You could also use NamedTemporaryFile(remove=False) here, which also works
> > as a context manager.
> 
> Doesn't NamedTemporaryFile leave the file open, which would prevent the
> subprocess from reading it on windows?

So, it's possible to .close the NamedTemporaryFile and not have the problem, but the whole with block still needs to be in a try/finally block to os.remove the file anyways. So all in all, that doesn't bring much advantage, and increases the indentation level.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3370e2233f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8cf0fe7c810

Comment 18

[Christian Holler (:decoder)]

Fwiw, this breaks building with afl-clang-fast wrapper.

With the changes landed here, it seems that the build system tries to set HOST_CXX to afl-clang++-fast although I specified CXX=afl-clang-fast++. I think using HOST_CXX=CXX is much more reasonable than doing some kind of string replace on CC (!) to figure out HOST_CXX and that's what it looks like to me. Correct me if I'm wrong.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3370e2233f6
https://hg.mozilla.org/mozilla-central/rev/f8cf0fe7c810

Comment 20

[Jonathan Kew [:jfkthame]]

f8cf0fe7c810 seems to have broken the build for me on Windows. It dies pretty quickly with a Python error, complaining that it hasn't heard of 'normcase', at the line

    if os.path.normcase(find_program(compiler)) != os.path.normcase(
            full_path):

I just tried updating to the latest mozilla-build, but that didn't help....

Comment 21

[(not currently active) Ted Mielczarek]

That should be fixed on inbound by bug 1264831.