Closed Bug 1259382 Opened 4 years ago Closed 4 years ago

Move CC/CXX/etc. to moz.configure

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

No description provided.
Depends on: 1259960
Depends on: 1260624
Depends on: 1260636
Depends on: 1260647
Depends on: 1260996
Depends on: 1260998
Depends on: 1261263
Depends on: 1261359
Depends on: 1262020
Depends on: 1262019
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/
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/
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.
Blocks: 1254987
Blocks: 1257448
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
Attachment #8738109 - Flags: review?(ted) → review+
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
Attachment #8738110 - Flags: review?(ted) → review+
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
Attachment #8738111 - Flags: review?(ted) → review+
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
Attachment #8738112 - Flags: review?(ted) → review+
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.
Keywords: leave-open
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?
Attachment #8738113 - Flags: review?(ted) → review+
(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
Blocks: 1264482
(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.
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.
Depends on: 1264609
Depends on: 1264919
Depends on: 1264959
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....
That should be fixed on inbound by bug 1264831.
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
No longer depends on: 1264609
Depends on: 1266620
No longer depends on: 1264919
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.