Closed Bug 973142 Opened 6 years ago Closed 6 years ago

Get rid of the MOZILLA_INTERNAL_API makefile variable

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → ehsan
Attachment #8376645 - Flags: review?(mshal)
Attachment #8376645 - Flags: review?(mh+mozilla)
Attachment #8376645 - Flags: review?(gps)
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
Attachment #8376645 - Flags: review?(mshal)
Attachment #8376645 - Flags: review?(mh+mozilla)
Attachment #8376645 - Flags: review?(gps)
Attachment #8376645 - Flags: review+
(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.
https://hg.mozilla.org/mozilla-central/rev/6a647f7bafe2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.