883350 - move SDK_HEADERS to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[jarmstrong]

Attachment: move SDK_HEADERS to mozbuild (logic)

Comment 2

[jarmstrong]

Attachment: move SDK_HEADERS to mozbuild (logic)

Comment 3

[jarmstrong]

Comment on attachment 766719 [details] [diff] [review]
move SDK_HEADERS to mozbuild (logic)

Add a passthrough conversion for SDK_HEADERS.

Comment 4

[Benjamin Smedberg]

Since SDK_HEADERS is just a synonym for EXPORTS (since Firefox 5 or so), please just remove it entirely and migrate those files to EXPORTS.

Comment 5

[jarmstrong]

Attachment: move SDK_HEADERS to mozbuild (file batch #1)

Comment 6

[jarmstrong]

Attachment: move SDK_HEADERS to mozbuild (file batch #1)

Convert SDK_HEADERS into EXPORTS +=.
Try results pending.

Comment 7

[Michael Shal [:mshal]]

Comment on attachment 766835 [details] [diff] [review]
move SDK_HEADERS to mozbuild (file batch #1)

> include $(topsrcdir)/config/rules.mk
>diff --git a/embedding/base/moz.build b/embedding/base/moz.build
>--- a/embedding/base/moz.build
>+++ b/embedding/base/moz.build
>@@ -8,8 +8,11 @@ XPIDL_SOURCES += [
>     'nsIDialogCreator.idl',
>     'nsIWindowCreator.idl',
>     'nsIWindowCreator2.idl',
>     'nsIWindowProvider.idl',
> ]
> 
> MODULE = 'embed_base'
> 
>+EXPORTS += [ # SDK_HEADERS
>+    'nsEmbedCID.h',
>+]

Since there is no longer any distinction between SDK_HEADERS and EXPORTS, I don't think we need to preserve the old variable name in the comment. Just remove the " # SDK_HEADERS" part:

EXPORTS += [
    'nsEmbedCID.h',
]

(occurs in all moz.build files, not just this one)

We'll also want to combine these EXPORTS with existing EXPORTS declarations (eg: xpcom/string/public/moz.build has 2 'EXPORTS +=' declarations next to each other) - do we have a bug on file for that already? Maybe this could be something to add to the style checker if possible (bug 774381).

Comment 8

[jarmstrong]

Attachment: move SDK_HEADERS to mozbuild (file batch #1)

Comment 9

[Joey Armstrong [:joey]]

Comment on attachment 768426 [details] [diff] [review]
move SDK_HEADERS to mozbuild (file batch #1)

Removed SDK_ comments.
Combined redundant EXPORT= declarations.
Try build job passed with the exception of leakstat on osx.
https://tbpl.mozilla.org/?tree=Try&rev=d87eaf348739

Run a test job on try and the patch should be ready for inbound.

Comment 10

[Joey Armstrong [:joey]]

Comment on attachment 768426 [details] [diff] [review]
move SDK_HEADERS to mozbuild (file batch #1)

b2g arm/emulator failure was due to a timeout in mochitest-6.

Comment 11

[Michael Shal [:mshal]]

Comment on attachment 768426 [details] [diff] [review]
move SDK_HEADERS to mozbuild (file batch #1)

Looks good to me!

Comment 12

[Joey Armstrong [:joey]]

Comment on attachment 768426 [details] [diff] [review]
move SDK_HEADERS to mozbuild (file batch #1)

another inbound push:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2199300dce27
committed changeset 137062:2199300dce27

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2199300dce27

Comment 14

[Joey Armstrong [:joey]]

Added leave-open whiteboard tag.  A patch to remove DISABLED_* will need to be submitted in a week or so.

Comment 15

[Joey Armstrong [:joey]]

Attachment: Cleanup/final patch for SDK_HEADERS conversion.

Comment 16

[Joey Armstrong [:joey]]

Comment on attachment 775949 [details] [diff] [review]
Cleanup/final patch for SDK_HEADERS conversion.

Final patch to remove DISABLED_ and SDK_HEADERS can be crossed off the todo list.

Comment 17

[Michael Shal [:mshal]]

Comment on attachment 775949 [details] [diff] [review]
Cleanup/final patch for SDK_HEADERS conversion.

Looks like you missed a few directories?

./xpcom/string/public/Makefile.in:DISABLED_SDK_HEADERS	=				\
./xpcom/Makefile.in:DISABLED_SDK_HEADERS	= xpcom-config.h
./xpcom/glue/standalone/Makefile.in:DISABLED_SDK_HEADERS     = \
./xpcom/io/Makefile.in:DISABLED_SDK_HEADERS =  \

Otherwise it looks fine and is ready to land, just doesn't look like the "final" patch :)

Comment 18

[Joey Armstrong [:joey]]

(In reply to Michael Shal [:mshal] from comment #17)
> Comment on attachment 775949 [details] [diff] [review]
> Cleanup/final patch for SDK_HEADERS conversion.
> 
> Looks like you missed a few directories?
> 
> ./xpcom/string/public/Makefile.in:DISABLED_SDK_HEADERS	=				\
> ./xpcom/Makefile.in:DISABLED_SDK_HEADERS	= xpcom-config.h
> ./xpcom/glue/standalone/Makefile.in:DISABLED_SDK_HEADERS     = \
> ./xpcom/io/Makefile.in:DISABLED_SDK_HEADERS =  \
> 
> Otherwise it looks fine and is ready to land, just doesn't look like the
> "final" patch :)

hrmph now how were those omitted

Comment 19

[Joey Armstrong [:joey]]

Attachment: Cleanup/final patch for SDK_HEADERS conversion.

Comment 20

[Joey Armstrong [:joey]]

Comment on attachment 776363 [details] [diff] [review]
Cleanup/final patch for SDK_HEADERS conversion.

Added the stragglers.

Comment 21

[Michael Shal [:mshal]]

Comment on attachment 776363 [details] [diff] [review]
Cleanup/final patch for SDK_HEADERS conversion.

Looks good now!

Comment 22

[Joey Armstrong [:joey]]

Attachment: Cleanup/final patch for SDK_HEADERS conversion.

Comment 23

[Joey Armstrong [:joey]]

Comment on attachment 777956 [details] [diff] [review]
Cleanup/final patch for SDK_HEADERS conversion.

rebase, r=mshal carried forward

Comment 24

[Joey Armstrong [:joey]]

Todo:

cleanup - embedding/base/Makefile.in: DISABLED_SDK_HEADERS =
Add SDK_HEADERS to the _MOZBUILD_EXTERNAL_VARIABLES list.

Comment 25

[Joey Armstrong [:joey]]

Attachment: SDK_HEADERS conversion cleanup.

Comment 26

[Joey Armstrong [:joey]]

Comment on attachment 796142 [details] [diff] [review]
SDK_HEADERS conversion cleanup.

Add SDK_HEADERS to the list of makefile rejected variables.

Comment 27

[Michael Shal [:mshal]]

Comment on attachment 796142 [details] [diff] [review]
SDK_HEADERS conversion cleanup.

Looks good!

We can also get rid of this block in rules.mk, right?

ifdef SDK_HEADERS
_EXTRA_EXPORTS := $(filter-out $(EXPORTS),$(SDK_HEADERS))
EXPORTS += $(_EXTRA_EXPORTS)
endif

Comment 28

[Joey Armstrong [:joey]]

(In reply to Michael Shal [:mshal] from comment #27)
> Comment on attachment 796142 [details] [diff] [review]
> SDK_HEADERS conversion cleanup.
> 
> Looks good!
> 
> We can also get rid of this block in rules.mk, right?
> 
> ifdef SDK_HEADERS
> _EXTRA_EXPORTS := $(filter-out $(EXPORTS),$(SDK_HEADERS))
> EXPORTS += $(_EXTRA_EXPORTS)
> endif

Probably yes, to be pruned shortly.

Try job: http://tbpl.mozilla.org/?tree=Try&rev=b68765e872d3

jetpack failure due to win32file import failure on win64.

Comment 29

[Joey Armstrong [:joey]]

Attachment: SDK_HEADERS conversion cleanup.

Comment 30

[Joey Armstrong [:joey]]

Attachment: Move SDK_HEADERS to mozbuild: cleanup patch

Refresh patch, combine patches adding SDK_HEADESR on the variable rejection list and remove lingering rules.mk logic.

Comment 31

[Joey Armstrong [:joey]]

(In reply to Joey Armstrong [:joey] from comment #30)
> Created attachment 801728 [details] [diff] [review]
> Move SDK_HEADERS to mozbuild: cleanup patch
> 
> Refresh patch, combine patches adding SDK_HEADESR on the variable rejection
> list and remove lingering rules.mk logic.

Try job: https://tbpl.mozilla.org/?tree=Try&rev=02b7921c4def

Comment 32

[Michael Shal [:mshal]]

Comment on attachment 801728 [details] [diff] [review]
Move SDK_HEADERS to mozbuild: cleanup patch

Looks good - thanks!

Comment 33

[Joey Armstrong [:joey]]

Comment on attachment 801728 [details] [diff] [review]
Move SDK_HEADERS to mozbuild: cleanup patch

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

Inbound push: https://hg.mozilla.org/integration/mozilla-inbound/rev/726355fba9b6

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/726355fba9b6