Closed Bug 976885 Opened 7 years ago Closed 7 years ago

Port RCFLAGS to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(2 files)

No description provided.
Assignee: nobody → ehsan
Attachment #8381827 - Flags: review?(mshal)
Attachment #8381827 - Flags: review?(mh+mozilla)
Attachment #8381827 - Flags: review?(gps)
Comment on attachment 8381827 [details] [diff] [review]
Port RCFLAGS to moz.build

Review of attachment 8381827 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/moz.build
@@ +58,5 @@
> +    RCFLAGS += ['-DMOZ_PHOENIX']
> +    if CONFIG['GNU_CC']:
> +        RCFLAGS += ['--include-dir ' + SRCDIR]
> +    else:
> +        RCFLAGS += ['-I' + SRCDIR]

The --include-dir/-I business sounds like it should just be part of the  command by default, instead of adding the same snippet over and over.

As for the remaining RCFLAGS, which are all -D flags, can't we just use DEFINES?
Attachment #8381827 - Flags: review?(mshal)
Attachment #8381827 - Flags: review?(mh+mozilla)
Attachment #8381827 - Flags: review?(gps)
Attachment #8381827 - Flags: review-
In fact, the command already uses INCLUDES and DEFINES... (although that filter-out should probably be on both ends)

ifdef GNU_CC
        $(RC) $(RCFLAGS) $(filter-out -U%,$(DEFINES)) $(INCLUDES:-I%=--include-dir %) $(OUTOPTION)$@ $(_VPATH_SRCS)
else
        $(RC) $(RCFLAGS) -r $(DEFINES) $(INCLUDES) $(OUTOPTION)$@ $(_VPATH_SRCS)
endif
(and INCLUDES should already have -I$(srcdir) in it)
So are you saying that I should just remove the variable?
Flags: needinfo?(mh+mozilla)
Yes, and replace the few RCFLAGS += -Dsomething with DEFINES[something] = True

And, obviously, check that this works properly.
Flags: needinfo?(mh+mozilla)
Attachment #8381901 - Flags: review?(mshal)
Attachment #8381901 - Flags: review?(mh+mozilla)
Attachment #8381901 - Flags: review?(gps)
Comment on attachment 8381901 [details] [diff] [review]
Port RCFLAGS to moz.build

Review of attachment 8381901 [details] [diff] [review]:
-----------------------------------------------------------------

r+ provided the answer to the question below is yes.

::: webapprt/win/Makefile.in
@@ -36,5 @@
> -else
> -RCFLAGS += --include-dir $(srcdir)
> -endif
> -ifdef DEBUG
> -RCFLAGS += -DDEBUG

Do we really have -DDEBUG in DEFINES on debug builds?
Attachment #8381901 - Flags: review?(mshal)
Attachment #8381901 - Flags: review?(mh+mozilla)
Attachment #8381901 - Flags: review?(gps)
Attachment #8381901 - Flags: review+
(In reply to Mike Hommey [:glandium] from comment #8)
> Comment on attachment 8381901 [details] [diff] [review]
> Port RCFLAGS to moz.build
> 
> Review of attachment 8381901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ provided the answer to the question below is yes.
> 
> ::: webapprt/win/Makefile.in
> @@ -36,5 @@
> > -else
> > -RCFLAGS += --include-dir $(srcdir)
> > -endif
> > -ifdef DEBUG
> > -RCFLAGS += -DDEBUG
> 
> Do we really have -DDEBUG in DEFINES on debug builds?

Hmm, I thought we did, but it seems like we don't.  I'll fix this when landing.
https://hg.mozilla.org/mozilla-central/rev/58053e39e2d4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.