Closed Bug 1265627 Opened 4 years ago Closed 4 years ago

Move compiler version check 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

(8 files)

58 bytes, text/x-review-board-request
ted
: review+
Details
58 bytes, text/x-review-board-request
ted
: review+
Details
58 bytes, text/x-review-board-request
ted
: review+
Details
58 bytes, text/x-review-board-request
ted
: review+
Details
58 bytes, text/x-review-board-request
ted
: review+
Details
58 bytes, text/x-review-board-request
ted
: review+
Details
58 bytes, text/x-review-board-request
ted
: review+
Details
58 bytes, text/x-review-board-request
ted
: review+
Details
No description provided.
While this is not immediately useful, it will be with the next changes.

Review commit: https://reviewboard.mozilla.org/r/47397/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47397/
Attachment #8742656 - Flags: review?(ted)
Attachment #8742657 - Flags: review?(ted)
Attachment #8742658 - Flags: review?(ted)
Attachment #8742659 - Flags: review?(ted)
Attachment #8742660 - Flags: review?(ted)
Attachment #8742661 - Flags: review?(ted)
Attachment #8742662 - Flags: review?(ted)
Attachment #8742663 - Flags: review?(ted)
Its only use is in buildconfig.html, and configure guarantees it's
always the same version as CC_VERSION, so just use that.

Review commit: https://reviewboard.mozilla.org/r/47405/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47405/
Comment on attachment 8742661 [details]
MozReview Request: Bug 1265627 - Remove now useless version-related assignments from old-configure. r?ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47407/diff/1-2/
Comment on attachment 8742662 [details]
MozReview Request: Bug 1265627 - Force clang-cl MSVC emulation from moz.configure. r?ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47409/diff/1-2/
Comment on attachment 8742663 [details]
MozReview Request: Bug 1265627 - Prettify the MSVC version. r?ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47411/diff/1-2/
Attachment #8742656 - Flags: review?(ted) → review+
Comment on attachment 8742656 [details]
MozReview Request: Bug 1265627 - Check that using the compiler flags we add does what they are intended to do. r?ted

https://reviewboard.mozilla.org/r/47397/#review44269

::: build/moz.configure/toolchain.configure:473
(Diff revision 1)
> +            type, version, more_flags = check_compiler(
> +                wrapper + [compiler] + flags, language)
> +
> +        if more_flags:
> +            raise FatalCheckError(
> +                'Unknown compiler or compiler not supported.')

I might make this a bit more verbose, describing exactly what unexpected circumstance we wound up in, like maybe "The compiler configuration didn't return consistent results"?
Comment on attachment 8742657 [details]
MozReview Request: Bug 1265627 - Don't disable strict aliasing for GCC < 4.5, we don't support those versions anyways. r?ted

https://reviewboard.mozilla.org/r/47399/#review44271

I love how much dead code you wind up removing in this process!
Attachment #8742657 - Flags: review?(ted) → review+
Comment on attachment 8742658 [details]
MozReview Request: Bug 1265627 - Remove leftovers from bug 1167005. r?ted

https://reviewboard.mozilla.org/r/47401/#review44273
Attachment #8742658 - Flags: review?(ted) → review+
Attachment #8742659 - Flags: review?(ted) → review+
Comment on attachment 8742659 [details]
MozReview Request: Bug 1265627 - Move compiler version checks to moz.configure. r?ted

https://reviewboard.mozilla.org/r/47403/#review44277

::: build/moz.configure/toolchain.configure:230
(Diff revision 1)
>          %COMPILER msvc
>          %VERSION _MSC_FULL_VER
>          #endif
>          #elif defined(__clang__)
>          %COMPILER clang
> +        #  if !__cplusplus || __cpp_static_assert

I gather the `__cplusplus` bit here is to handle version-testing for the clang C compiler, which we weren't previously doing? Does this give us a usable version number in that case?

::: build/moz.configure/toolchain.configure:271
(Diff revision 1)
>      cplusplus = int(data.get('cplusplus', '0L').rstrip('L'))
>      stdc_version = int(data.get('STDC_VERSION', '0L').rstrip('L'))
>  
>      return namespace(
>          type=CompilerType(type),
> -        version=data['VERSION'],
> +        version=data.get('VERSION'),

Can we return a `Version` here or is that too hard for some reason?

::: build/moz.configure/toolchain.configure:506
(Diff revision 1)
> +        if type == 'clang' and not version:
> +            raise FatalCheckError(
> +                'Only clang/llvm 3.4 or newer is supported.')
> +
> +        if type == 'msvc':
> +            if version < '180030723' or (version > '19' and

It would be nice to split up the msvc version into bits and put it in a `Version` to make these checks nicer.
Comment on attachment 8742660 [details]
MozReview Request: Bug 1265627 - Remove the CXX_VERSION AC_SUBST. r?ted

https://reviewboard.mozilla.org/r/47405/#review44279
Attachment #8742660 - Flags: review?(ted) → review+
Comment on attachment 8742661 [details]
MozReview Request: Bug 1265627 - Remove now useless version-related assignments from old-configure. r?ted

https://reviewboard.mozilla.org/r/47407/#review44281
Attachment #8742661 - Flags: review?(ted) → review+
Comment on attachment 8742662 [details]
MozReview Request: Bug 1265627 - Force clang-cl MSVC emulation from moz.configure. r?ted

https://reviewboard.mozilla.org/r/47409/#review44283

::: build/moz.configure/toolchain.configure:316
(Diff revision 2)
> +    # cl.exe.
> +    if info.type == 'clang-cl' and info.version != '180030723':
> +        # Those flags are direct clang-cl flags that don't need -Xclang, add
> +        # them directly.
> +        flags.append('-fms-compatibility-version=18.00.30723')
> +        flags.append('-fallback')

Well that does work out nicely! :)
Attachment #8742662 - Flags: review?(ted) → review+
Attachment #8742663 - Flags: review?(ted) → review+
Comment on attachment 8742663 [details]
MozReview Request: Bug 1265627 - Prettify the MSVC version. r?ted

https://reviewboard.mozilla.org/r/47411/#review44285

::: build/moz.configure/toolchain.configure:524
(Diff revision 2)
>          if type == 'clang' and not version:
>              raise FatalCheckError(
>                  'Only clang/llvm 3.4 or newer is supported.')
>  
>          if type == 'msvc':
> -            if version < '180030723' or (version > '19' and
> +            ver = Version(version)

Oh, that's nicer. I still think you should move the Version wrapping up a bit.

::: old-configure.in:350
(Diff revision 2)
>  
>          changequote(,)
>          _MSVC_VER_FILTER='s|.*[^!-~]([0-9]+\.[0-9]+\.[0-9]+(\.[0-9]+)?).*|\1|p'
>          changequote([,])
>  
> -        _MSC_VER=`echo ${CC_VERSION} | cut -c 1-4`
> +        _MSC_VER=`echo ${CC_VERSION} | cut -c 1-2,4-5`

Why not just move this to moz.configure with an old_configure_assignment while you're here?
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #12)
> Comment on attachment 8742656 [details]
> MozReview Request: Bug 1265627 - Check that using the compiler flags we add
> does what they are intended to do. r?ted
> 
> https://reviewboard.mozilla.org/r/47397/#review44269
> 
> ::: build/moz.configure/toolchain.configure:473
> (Diff revision 1)
> > +            type, version, more_flags = check_compiler(
> > +                wrapper + [compiler] + flags, language)
> > +
> > +        if more_flags:
> > +            raise FatalCheckError(
> > +                'Unknown compiler or compiler not supported.')
> 
> I might make this a bit more verbose, describing exactly what unexpected
> circumstance we wound up in, like maybe "The compiler configuration didn't
> return consistent results"?

Let's keep it like this for now. I have something else in my queue that will change it.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15)
> Comment on attachment 8742659 [details]
> MozReview Request: Bug 1265627 - Move compiler version checks to
> moz.configure. r?ted
> 
> https://reviewboard.mozilla.org/r/47403/#review44277
> 
> ::: build/moz.configure/toolchain.configure:230
> (Diff revision 1)
> >          %COMPILER msvc
> >          %VERSION _MSC_FULL_VER
> >          #endif
> >          #elif defined(__clang__)
> >          %COMPILER clang
> > +        #  if !__cplusplus || __cpp_static_assert
> 
> I gather the `__cplusplus` bit here is to handle version-testing for the
> clang C compiler, which we weren't previously doing? Does this give us a
> usable version number in that case?

Not one that can be usefully used in version tests, but at least it can be displayed to the user.
https://reviewboard.mozilla.org/r/47411/#review44285

> Why not just move this to moz.configure with an old_configure_assignment while you're here?

Will do in a followup.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.