Cleanup the A11Y_LOG Makefile defines

RESOLVED FIXED in mozilla30

Status

RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: emorley, Assigned: Ehsan)

Tracking

Trunk
mozilla30

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(In reply to Ed Morley [:edmorley UTC+1] from comment #3)
> gps, the patterns used in comment 2 seem suboptimal - is there a better way
> for the a11y team to set this up?  Also is the preferred way to have the
> define set in configure or in the individual makefiles?

(In reply to Gregory Szorc [:gps] from comment #9)
> We should keep one of these bugs open to track a longer-term solution.

See bug 932206 comment 2 and/or:
http://mxr.mozilla.org/mozilla-central/search?string=A11Y_LOG
I have a patch that just derecursifies all of accessible/src that should just about be green.
Assignee: nobody → trev.saunders
Created attachment 828855 [details] [diff] [review]
patch

I kept the stuff in accessible/src/jsat  seperate intentionally since unlike the rest of the stuff in accessible/src its not c++ and really is its own module.
Attachment #828855 - Flags: review?(mshal)
Comment on attachment 828855 [details] [diff] [review]
patch

The goal of this bug is to get rid of the duplicated DEFINES += -DA11Y_LOG and the MOZ_UPDATE_CHANNEL logic, correct? I don't think putting the entire build configuration into one file is the right approach for this. Instead the common bits should be shared in a common file that each directory uses. That way we remove the redundancy, but still maintain the ability to have each directory define how to build the things in that directory.

Specifically, I think we meet the goals of this bug by just moving the A11Y_LOG DEFINES into accessible/src/shared.mozbuild:

if a11y_log:
    DEFINES['A11Y_LOG'] = True

And then just make sure shared.mozbuild is included by the sub-directories.
Attachment #828855 - Flags: review?(mshal) → review-
(In reply to Michael Shal [:mshal] from comment #3)
> Comment on attachment 828855 [details] [diff] [review]
> patch
> 
> The goal of this bug is to get rid of the duplicated DEFINES += -DA11Y_LOG
> and the MOZ_UPDATE_CHANNEL logic, correct? I don't think putting the entire
> build configuration into one file is the right approach for this. Instead

Well, it certainly isn't the minimal change, but it was an excuse to do something I've wanted to do for a while namely reduce the dupplication between all the Makefiles / moz.build files in accessible/src.

> the common bits should be shared in a common file that each directory uses.
> That way we remove the redundancy, but still maintain the ability to have
> each directory define how to build the things in that directory.

I don't think its particularly useful to have a bijection between directories with stuff that get built and moz.build files, I think it makes more sense to have something closer to one moz.build file per module.

> Specifically, I think we meet the goals of this bug by just moving the
> A11Y_LOG DEFINES into accessible/src/shared.mozbuild:
> 
> if a11y_log:
>     DEFINES['A11Y_LOG'] = True
> 
> And then just make sure shared.mozbuild is included by the sub-directories.

if your only goal is to fix this bug yes, on the other hand that's not my only goal, so I'm doing something else :)
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> (In reply to Michael Shal [:mshal] from comment #3)
> > Comment on attachment 828855 [details] [diff] [review]
> > patch
> > 
> > The goal of this bug is to get rid of the duplicated DEFINES += -DA11Y_LOG
> > and the MOZ_UPDATE_CHANNEL logic, correct? I don't think putting the entire
> > build configuration into one file is the right approach for this. Instead
> 
> Well, it certainly isn't the minimal change, but it was an excuse to do
> something I've wanted to do for a while namely reduce the dupplication
> between all the Makefiles / moz.build files in accessible/src.

I don't have any objections to reducing the duplication in these Makefiles/moz.build files :).

> 
> > the common bits should be shared in a common file that each directory uses.
> > That way we remove the redundancy, but still maintain the ability to have
> > each directory define how to build the things in that directory.
> 
> I don't think its particularly useful to have a bijection between
> directories with stuff that get built and moz.build files, I think it makes
> more sense to have something closer to one moz.build file per module.

Why? IMHO looking at the results, having a single accessible/src/moz.build file is harder to read & understand as compared to having one per directory.
I agree with the other Mike.
(In reply to Michael Shal [:mshal] from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > (In reply to Michael Shal [:mshal] from comment #3)
> > > Comment on attachment 828855 [details] [diff] [review]
> > > patch
> > > 
> > > The goal of this bug is to get rid of the duplicated DEFINES += -DA11Y_LOG
> > > and the MOZ_UPDATE_CHANNEL logic, correct? I don't think putting the entire
> > > build configuration into one file is the right approach for this. Instead
> > 
> > Well, it certainly isn't the minimal change, but it was an excuse to do
> > something I've wanted to do for a while namely reduce the dupplication
> > between all the Makefiles / moz.build files in accessible/src.
> 
> I don't have any objections to reducing the duplication in these
> Makefiles/moz.build files :).

good :)

> > > the common bits should be shared in a common file that each directory uses.
> > > That way we remove the redundancy, but still maintain the ability to have
> > > each directory define how to build the things in that directory.
> > 
> > I don't think its particularly useful to have a bijection between
> > directories with stuff that get built and moz.build files, I think it makes
> > more sense to have something closer to one moz.build file per module.
> 
> Why? IMHO looking at the results, having a single accessible/src/moz.build
> file is harder to read & understand as compared to having one per directory.

huh, imho its easier, I'm sort of curious why you think otherwise.

(In reply to Mike Hommey [:glandium] from comment #6)
> I agree with the other Mike.

Well, I came across some code in layout/base/nsPresShell.cpp that used A11Y_LOG the other day  so maybe I should just suck it up and add a configure arg :/  Although adding a configure arg only a couple of people are every likely to use feels dubious.
(Assignee)

Updated

5 years ago
Assignee: trev.saunders → ehsan
(Assignee)

Updated

5 years ago
Blocks: 928196
(Assignee)

Updated

5 years ago
Attachment #828855 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 8378047 [details] [diff] [review]
Move A11Y_LOG to configure; r=glandium
(Assignee)

Updated

5 years ago
Attachment #8378047 - Flags: review?(mh+mozilla)
Comment on attachment 8378047 [details] [diff] [review]
Move A11Y_LOG to configure; r=glandium

>+if test "$A11Y_LOG" != 0; then
>+    AC_DEFINE(A11Y_LOG)

I'm pretty sure that's wrong and you want something like the other MOZ_ARG_ENABLE_BOOL things in configure.in
(Assignee)

Comment 10

5 years ago
(In reply to comment #9)
> Comment on attachment 8378047 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8378047
> Move A11Y_LOG to configure; r=glandium
> 
> >+if test "$A11Y_LOG" != 0; then
> >+    AC_DEFINE(A11Y_LOG)
> 
> I'm pretty sure that's wrong and you want something like the other
> MOZ_ARG_ENABLE_BOOL things in configure.in

Wrong how?  I'm just trying to preserve the existing behavior, not change anything.
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #10)
> (In reply to comment #9)
> > Comment on attachment 8378047 [details] [diff] [review]
> >   --> https://bugzilla.mozilla.org/attachment.cgi?id=8378047
> > Move A11Y_LOG to configure; r=glandium
> > 
> > >+if test "$A11Y_LOG" != 0; then
> > >+    AC_DEFINE(A11Y_LOG)
> > 
> > I'm pretty sure that's wrong and you want something like the other
> > MOZ_ARG_ENABLE_BOOL things in configure.in
> 
> Wrong how?  I'm just trying to preserve the existing behavior, not change
> anything.

So actually you don't need to add a configure option though that would be nice.

What you do need to do to preserve behavior is move the logic in accessible/src/defs.mk and accessible/src/shared.mozbuild to configure and then change if a11y_log in accessible/src/base/moz.build to if CONFIG['a11y_log']: and add if CONFIG['a11y_log']: DEFINES += ... to each of the moz.build files under accessible/src

fwiw I was planning to get back to this eventually when I have some time.
Comment on attachment 8378047 [details] [diff] [review]
Move A11Y_LOG to configure; r=glandium

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

::: configure.in
@@ +8694,5 @@
>  fi
>  
> +if test "$A11Y_LOG" != 0; then
> +    AC_DEFINE(A11Y_LOG)
> +fi

accessible/src/defs.mk needs to be transposed here.
Attachment #8378047 - Flags: review?(mh+mozilla) → review-
(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> and add if CONFIG['a11y_log']: DEFINES += ... to each of
> the moz.build files under accessible/src

Why not just use AC_DEFINE?
(In reply to Mike Hommey [:glandium] from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > and add if CONFIG['a11y_log']: DEFINES += ... to each of
> > the moz.build files under accessible/src
> 
> Why not just use AC_DEFINE?

because apparently I'm not thinking tonight (though in my defense its 23:30)
(Assignee)

Comment 15

5 years ago
Created attachment 8378073 [details] [diff] [review]
Move A11Y_LOG to configure; r=glandium
(Assignee)

Updated

5 years ago
Attachment #8378047 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8378073 - Flags: review?(mh+mozilla)
(Assignee)

Updated

5 years ago
Attachment #8378073 - Attachment is obsolete: true
Attachment #8378073 - Flags: review?(mh+mozilla)
(Assignee)

Comment 16

5 years ago
Created attachment 8378074 [details] [diff] [review]
Move A11Y_LOG to configure; r=glandium
(Assignee)

Updated

5 years ago
Attachment #8378074 - Flags: review?(mh+mozilla)
Comment on attachment 8378074 [details] [diff] [review]
Move A11Y_LOG to configure; r=glandium

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

::: configure.in
@@ +8692,5 @@
>      AC_DEFINE_UNQUOTED(ATK_MINOR_VERSION, $ATK_MINOR_VERSION)
>      AC_DEFINE_UNQUOTED(ATK_REV_VERSION, $ATK_REV_VERSION)
>  fi
>  
> +A11Y_LOG=0

Remove this

@@ +8703,5 @@
> +*)
> +    A11Y_LOG=1
> +    ;;
> +esac
> +if test "$A11Y_LOG" != 0; then

and replace with test -n "$A11Y_LOG"

That will allow A11Y_LOG to be set in mozconfig if anyone wants to.

@@ +8704,5 @@
> +    A11Y_LOG=1
> +    ;;
> +esac
> +if test "$A11Y_LOG" != 0; then
> +    AC_SUBST(A11Y_LOG)

Move this outside the if. People would easily think AC_SUBST depends on the condition when it doesn't, contrary to AC_DEFINE.
Attachment #8378074 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 18

5 years ago
(In reply to comment #17)
> @@ +8704,5 @@
> > +    A11Y_LOG=1
> > +    ;;
> > +esac
> > +if test "$A11Y_LOG" != 0; then
> > +    AC_SUBST(A11Y_LOG)
> 
> Move this outside the if. People would easily think AC_SUBST depends on the
> condition when it doesn't, contrary to AC_DEFINE.

m4/autoconf is quite a gem! ;-)
https://hg.mozilla.org/mozilla-central/rev/eac78e505370
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

Updated

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