It's just as easy to directly set the preprocessor macro in the moz.build files. Using this variable doesn't really buy us anything.

Comment on attachment 8376645 [details] [diff] [review] Get rid of the MOZILLA_INTERNAL_API makefile variable Review of attachment 8376645 [details] [diff] [review]: ----------------------------------------------------------------- Please remove all the useless DEFINES. ::: intl/unicharutil/util/internal/moz.build @@ +14,5 @@ > '..', > '../../src', > ] > > +DEFINES['MOZILLA_INTERNAL_API'] = True This is unnecessary because FINAL_LIBRARY=xul, which translate as LIBXUL_LIBRARY being defined. ::: profile/dirserviceprovider/src/moz.build @@ +12,5 @@ > > # we don't want the shared lib > FORCE_STATIC_LIB = True > + > +DEFINES['MOZILLA_INTERNAL_API'] = True Nothing is using this lib afaict. Might as well just remove profile/dirserviceprovider. ::: rdf/tests/dsds/moz.build @@ +17,5 @@ > 'DataSourceViewer.xul', > 'DataSourceViewer.css', > ] > + > +DEFINES['MOZILLA_INTERNAL_API'] = True Nothing is using that lib, might as well kill it. ::: rdf/util/src/internal/moz.build @@ +9,5 @@ > SOURCES += rdf_util_src_cppsrcs > > FINAL_LIBRARY = 'xul' > + > +DEFINES['MOZILLA_INTERNAL_API'] = True FINAL_LIBRARY=xul implies this define. ::: services/crypto/component/moz.build @@ +19,5 @@ > FAIL_ON_WARNINGS = True > > FINAL_LIBRARY = 'xul' > + > +DEFINES['MOZILLA_INTERNAL_API'] = True Likewise ::: toolkit/components/feeds/moz.build @@ +25,5 @@ > 'FeedProcessor.js', > 'FeedProcessor.manifest', > ] > + > +DEFINES['MOZILLA_INTERNAL_API'] = True There is no C++ code in this directory, and nothing preprocessed. ::: toolkit/components/url-classifier/tests/moz.build @@ +9,5 @@ > XPCSHELL_TESTS_MANIFESTS += ['unit/xpcshell.ini'] > > JAR_MANIFESTS += ['jar.mn'] > + > +DEFINES['MOZILLA_INTERNAL_API'] = True Comment this, it's useless without C++ code. ::: toolkit/library/moz.build @@ +19,5 @@ > '/widget/windows', > '/xpcom/base', > ] > > +DEFINES['MOZILLA_INTERNAL_API'] = True This should be implied but isn't. Please make LIBRARY_NAME=xul mean LIBXUL_LIBRARY too like FINAL_LIBRARY does. ::: xpcom/base/moz.build @@ +157,5 @@ > '../build', > '/xpcom/ds', > ] > > +DEFINES['MOZILLA_INTERNAL_API'] = True FINAL_LIBRARY is xpcom_core, and xpcom_core's FINAL_LIBRARY is xul, so this is implied. ::: xpcom/build/moz.build @@ +71,5 @@ > FINAL_LIBRARY = 'xul' > > DEFINES['_IMPL_NS_STRINGAPI'] = True > DEFINES['OMNIJAR_NAME'] = CONFIG['OMNIJAR_NAME'] > +DEFINES['MOZILLA_INTERNAL_API'] = True FINAL_LIBRARY is xul, it's implied. ::: xpcom/components/moz.build @@ +43,5 @@ > MSVC_ENABLE_PGO = True > > FINAL_LIBRARY = 'xpcom_core' > + > +DEFINES['MOZILLA_INTERNAL_API'] = True Like xpcom/base. ::: xpcom/io/moz.build @@ +123,5 @@ > > FINAL_LIBRARY = 'xpcom_core' > > +DEFINES['MOZILLA_INTERNAL_API'] = True > + Like xpcom/base. ::: xpcom/reflect/xptcall/src/md/unix/moz.build @@ +315,5 @@ > ] > > NO_PGO = True > + > +DEFINES['MOZILLA_INTERNAL_API'] = True Likewise ::: xpcom/reflect/xptcall/src/md/win32/moz.build @@ +45,5 @@ > if not CONFIG['GNU_CXX']: > # FIXME: bug 413019 > NO_PGO = True > + > +DEFINES['MOZILLA_INTERNAL_API'] = True Likewise ::: xpcom/string/src/moz.build @@ +30,5 @@ > MSVC_ENABLE_PGO = True > > FINAL_LIBRARY = 'xpcom_core' > + > +DEFINES['MOZILLA_INTERNAL_API'] = True Likewise

(In reply to Mike Hommey [:glandium] from comment #2) > Comment on attachment 8376645 [details] [diff] [review] > Get rid of the MOZILLA_INTERNAL_API makefile variable > > Review of attachment 8376645 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please remove all the useless DEFINES. > > ::: intl/unicharutil/util/internal/moz.build > @@ +14,5 @@ > > '..', > > '../../src', > > ] > > > > +DEFINES['MOZILLA_INTERNAL_API'] = True > > This is unnecessary because FINAL_LIBRARY=xul, which translate as > LIBXUL_LIBRARY being defined. Can we detect this and fail the build? > ::: profile/dirserviceprovider/src/moz.build > @@ +12,5 @@ > > > > # we don't want the shared lib > > FORCE_STATIC_LIB = True > > + > > +DEFINES['MOZILLA_INTERNAL_API'] = True > > Nothing is using this lib afaict. Might as well just remove > profile/dirserviceprovider. Some of it is definitely used. > ::: toolkit/library/moz.build > @@ +19,5 @@ > > '/widget/windows', > > '/xpcom/base', > > ] > > > > +DEFINES['MOZILLA_INTERNAL_API'] = True > > This should be implied but isn't. Please make LIBRARY_NAME=xul mean > LIBXUL_LIBRARY too like FINAL_LIBRARY does. Not sure what you mean. Will address this in a follow-up but please clarify.

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #3) > (In reply to Mike Hommey [:glandium] from comment #2) > > Comment on attachment 8376645 [details] [diff] [review] > > Get rid of the MOZILLA_INTERNAL_API makefile variable > > > > Review of attachment 8376645 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Please remove all the useless DEFINES. > > > > ::: intl/unicharutil/util/internal/moz.build > > @@ +14,5 @@ > > > '..', > > > '../../src', > > > ] > > > > > > +DEFINES['MOZILLA_INTERNAL_API'] = True > > > > This is unnecessary because FINAL_LIBRARY=xul, which translate as > > LIBXUL_LIBRARY being defined. > > Can we detect this and fail the build? We could > > ::: profile/dirserviceprovider/src/moz.build > > @@ +12,5 @@ > > > > > > # we don't want the shared lib > > > FORCE_STATIC_LIB = True > > > + > > > +DEFINES['MOZILLA_INTERNAL_API'] = True > > > > Nothing is using this lib afaict. Might as well just remove > > profile/dirserviceprovider. > > Some of it is definitely used. Only the standalone part, which doesn't need MOZILLA_INTERNAL_API. > > ::: toolkit/library/moz.build > > @@ +19,5 @@ > > > '/widget/windows', > > > '/xpcom/base', > > > ] > > > > > > +DEFINES['MOZILLA_INTERNAL_API'] = True > > > > This should be implied but isn't. Please make LIBRARY_NAME=xul mean > > LIBXUL_LIBRARY too like FINAL_LIBRARY does. > > Not sure what you mean. Will address this in a follow-up but please clarify. config/config.mk sets LIBXUL_LIBRARY when FINAL_LIBRARY from backend.mk is xul (which, in terms of moz.build is true for anything that indirectly leads to a FINAL_LIBRARY=xul, like FINAL_LIBRARY=xpcom_core). It doesn't set LIBXUL_LIBRARY for LIBRARY_NAME=xul.

I am 100% in favor of adding more checks to fail the build if something that doesn't make sense is present. Failing fast (during config.status) is preferred over failing during the build.

(In reply to Gregory Szorc [:gps] from comment #7) > I am 100% in favor of adding more checks to fail the build if something that > doesn't make sense is present. Failing fast (during config.status) is > preferred over failing during the build. Those are harmless. They're just useless.

(In reply to Mike Hommey [:glandium] from comment #8) > (In reply to Gregory Szorc [:gps] from comment #7) > > I am 100% in favor of adding more checks to fail the build if something that > > doesn't make sense is present. Failing fast (during config.status) is > > preferred over failing during the build. > > Those are harmless. They're just useless. I'm not sure what you are referring to. But I argue variables that do nothing in moz.build files aren't harmless because developers often think they do something. People waste time changing things they think will have an impact only to find out after much debugging that they don't actually do anything. We have the capability to prevent cargo culting and confusion: we should leverage it.

(In reply to Mike Hommey [:glandium] from comment #5) > > > ::: profile/dirserviceprovider/src/moz.build > > > @@ +12,5 @@ > > > > > > > > # we don't want the shared lib > > > > FORCE_STATIC_LIB = True > > > > + > > > > +DEFINES['MOZILLA_INTERNAL_API'] = True > > > > > > Nothing is using this lib afaict. Might as well just remove > > > profile/dirserviceprovider. > > > > Some of it is definitely used. > > Only the standalone part, which doesn't need MOZILLA_INTERNAL_API. Can you please tell me what you want to do here? Do you want me to remove MOZILLA_INTERNAL_API from the moz.build file? > > > ::: toolkit/library/moz.build > > > @@ +19,5 @@ > > > > '/widget/windows', > > > > '/xpcom/base', > > > > ] > > > > > > > > +DEFINES['MOZILLA_INTERNAL_API'] = True > > > > > > This should be implied but isn't. Please make LIBRARY_NAME=xul mean > > > LIBXUL_LIBRARY too like FINAL_LIBRARY does. > > > > Not sure what you mean. Will address this in a follow-up but please clarify. > > config/config.mk sets LIBXUL_LIBRARY when FINAL_LIBRARY from backend.mk is > xul (which, in terms of moz.build is true for anything that indirectly leads > to a FINAL_LIBRARY=xul, like FINAL_LIBRARY=xpcom_core). It doesn't set > LIBXUL_LIBRARY for LIBRARY_NAME=xul. OK, gotcha. Will file a follow-up for this.

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #10) > Can you please tell me what you want to do here? Do you want me to remove > MOZILLA_INTERNAL_API from the moz.build file? Remove the non-standalone part.

Filed bug 974216 and bug 974217.