Move CC/CXX/etc. to moz.configure

RESOLVED FIXED

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Assignee

Description

3 years ago
No description provided.
Assignee

Updated

3 years ago
Depends on: 1259960
Assignee

Updated

3 years ago
Depends on: 1260624
Assignee

Updated

3 years ago
Depends on: 1260636
Assignee

Updated

3 years ago
Depends on: 1260647
Assignee

Updated

3 years ago
Depends on: 1260996
Assignee

Updated

3 years ago
Depends on: 1260998
Assignee

Updated

3 years ago
Depends on: 1261263
Assignee

Updated

3 years ago
Depends on: 1261359
Assignee

Updated

3 years ago
Depends on: 1262020
Assignee

Updated

3 years ago
Depends on: 1262019
Assignee

Comment 4

3 years ago
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/
Assignee

Comment 5

3 years ago
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/
Assignee

Comment 6

3 years ago
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.

Updated

3 years ago
Blocks: 1254987
Assignee

Updated

3 years ago
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+
Assignee

Comment 11

3 years ago
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+
Assignee

Comment 15

3 years ago
(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
Assignee

Updated

3 years ago
Blocks: 1264482
Assignee

Comment 16

3 years ago
(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.
Assignee

Updated

3 years ago
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Assignee

Updated

3 years ago
No longer depends on: 1264609
Assignee

Updated

3 years ago
Depends on: 1266620
No longer depends on: 1264919

Updated

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