935548 - Cleanup the A11Y_LOG Makefile defines

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ed Morley [:emorley]]

(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

Comment 1

[Trevor Saunders (:tbsaunde)]

I have a patch that just derecursifies all of accessible/src that should just about be green.

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: 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.

Comment 3

[Michael Shal [: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.

Comment 4

[Trevor Saunders (:tbsaunde)]

(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 :)

Comment 5

[Michael Shal [:mshal]]

(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.

Comment 6

[Mike Hommey [:glandium]]

I agree with the other Mike.

Comment 7

[Trevor Saunders (:tbsaunde)]

(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.

Comment 8

[(no longer active)]

Attachment: Move A11Y_LOG to configure; r=glandium

Comment 9

[Trevor Saunders (:tbsaunde)]

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

Comment 10

[(no longer active)]

(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.

Comment 11

[Trevor Saunders (:tbsaunde)]

(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 12

[Mike Hommey [:glandium]]

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.

Comment 13

[Mike Hommey [:glandium]]

(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?

Comment 14

[Trevor Saunders (:tbsaunde)]

(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)

Comment 15

[(no longer active)]

Attachment: Move A11Y_LOG to configure; r=glandium

Comment 16

[(no longer active)]

Attachment: Move A11Y_LOG to configure; r=glandium

Comment 17

[Mike Hommey [:glandium]]

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.

Comment 18

[(no longer active)]

(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! ;-)

Comment 19

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eac78e505370

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eac78e505370