Closed
Bug 1265627
Opened 8 years ago
Closed 8 years ago
Move compiler version check to moz.configure
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
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.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47399/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47399/
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47401/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47401/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47403/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47403/
Assignee | ||
Comment 5•8 years ago
|
||
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/
Assignee | ||
Comment 6•8 years ago
|
||
Also simplify things around some remaining compiler version checks. Review commit: https://reviewboard.mozilla.org/r/47407/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47407/
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47409/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47409/
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47411/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47411/
Assignee | ||
Comment 9•8 years ago
|
||
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/
Assignee | ||
Comment 10•8 years ago
|
||
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/
Assignee | ||
Comment 11•8 years ago
|
||
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/
Updated•8 years ago
|
Attachment #8742656 -
Flags: review?(ted) → review+
Comment 12•8 years ago
|
||
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 13•8 years ago
|
||
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 14•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8742659 -
Flags: review?(ted) → review+
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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 17•8 years ago
|
||
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 18•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8742663 -
Flags: review?(ted) → review+
Comment 19•8 years ago
|
||
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?
Assignee | ||
Comment 20•8 years ago
|
||
(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.
Assignee | ||
Comment 21•8 years ago
|
||
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.
Comment 22•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/54715f7701ed https://hg.mozilla.org/integration/mozilla-inbound/rev/98351b5276d0 https://hg.mozilla.org/integration/mozilla-inbound/rev/3743c8668bed https://hg.mozilla.org/integration/mozilla-inbound/rev/a7802ffcbc4b https://hg.mozilla.org/integration/mozilla-inbound/rev/39a20595ffa2 https://hg.mozilla.org/integration/mozilla-inbound/rev/bb34c1ddc755 https://hg.mozilla.org/integration/mozilla-inbound/rev/c21f5aae0771 https://hg.mozilla.org/integration/mozilla-inbound/rev/ff37852da653
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/54715f7701ed https://hg.mozilla.org/mozilla-central/rev/98351b5276d0 https://hg.mozilla.org/mozilla-central/rev/3743c8668bed https://hg.mozilla.org/mozilla-central/rev/a7802ffcbc4b https://hg.mozilla.org/mozilla-central/rev/39a20595ffa2 https://hg.mozilla.org/mozilla-central/rev/bb34c1ddc755 https://hg.mozilla.org/mozilla-central/rev/c21f5aae0771 https://hg.mozilla.org/mozilla-central/rev/ff37852da653
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•