Closed Bug 557000 Opened 14 years ago Closed 14 years ago

mingw build does not set GCC_VERSION.

Categories

(Firefox Build System :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: jacek, Assigned: jacek)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch fixSplinter Review
I noticed that after mingw build breakage from bug 551254. The obvious fix is attached. There are, however, two things to consider:

- Setting GCC_VERSION in configure script seems more like a side effect of version-depended configurations and there are other platforms that don't set it. Should it be cleaned up?

= The patch from bug 551254 is the only code that uses GCC_VERSION in such way. Shouldn't it use things that seem more standard like GNU_CC?
Attachment #436862 - Flags: review?(ted.mielczarek)
Attachment #436862 - Attachment is patch: true
Blocks: 551254
Attachment #436862 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 436862 [details] [diff] [review]
fix

Yeah, that code probably should have used GNU_CXX. This is good for consistency anyway.
Assignee: nobody → jacek
Thanks for the review. I've attached a patch that removes use of GCC_VERSION. It does a bit more as it reverses the logic to 'not msvc' instead. I think that's a better solution.
Attachment #437811 - Flags: review?(ted.mielczarek)
Keywords: checkin-needed
The second patch rebased against current m-c.
Attachment #437811 - Attachment is obsolete: true
Attachment #441020 - Flags: review?(ted.mielczarek)
Attachment #437811 - Flags: review?(ted.mielczarek)
Comment on attachment 441020 [details] [diff] [review]
Don't use GCC_VERSION to detect GCC in makefiles. (rebased)

I'd rather have the block in config/Makefile.in explicitly check for ifdef GNU_CXX instead of assuming gcc if we're not using MSVC. I know there are people actively using stuff like SunCC and ICC, we should make sure we don't break them.
Attachment #441020 - Flags: review?(ted.mielczarek) → review-
Thanks for the review, I've attached a fixed patch.
Attachment #441020 - Attachment is obsolete: true
Attachment #442904 - Flags: review?(ted.mielczarek)
Attachment #442904 - Flags: review?(ted.mielczarek) → review+
http://hg.mozilla.org/mozilla-central/rev/ca141b029e79
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Thanks for landing, but the bug is not yet fixed. Attachment 436862 [details] [diff] also requires landing.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
oops, please next time specify in the whiteboard what needs to land
the missing push
http://hg.mozilla.org/mozilla-central/rev/5e68d39a2b4a
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Blocks: 802343
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: