369395 - Same headers in config should be installed in install and export rules

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

Attachment: patch

All headers in the config directory are not installed. Maybe they should.

See attached patch.

Comment 1

[Wan-Teh Chang]

Comment on attachment 254062 [details] [diff] [review]
patch

I think some of the headers in config, such as pathsub.h,
should not be installed.  Ben, what do you think?

Comment 2

[Mike Hommey [:glandium]]

Sorry, the title and explanation are actually misleading. I overlooked what the patche actually does.

The export rule installs files that are defined in the HEADERS variable in dist, while the install rule installs nsBuildID.h and mozilla-config.h, missing nsStaticComponents.h.

The rule for export uses $(HEADERS), while the rules in install names the files explicitely. The patch makes the install rule use $(HEADER) instead.

Comment 3

[Wan-Teh Chang]

Comment on attachment 254062 [details] [diff] [review]
patch

Ben, should the 'install' target of mozilla/config/Makefile.in install
the header file nsStaticComponents.h?

Comment 4

[Benjamin Smedberg]

Comment on attachment 254062 [details] [diff] [review]
patch

We should not be installing nsStaticComponents.h or nsBuildID.h. They are solely for use when building the app, and are not usable or useful externally.

Comment 5

[Mike Hommey [:glandium]]

So, let's reverse the question : are they needed in the sdk ? considering it's made out of dist, and these files end up in dist...

Comment 6

[Wan-Teh Chang]

Ben, could you answer Mike's question in comment 5?

Comment 7

[Benjamin Smedberg]

They should not end up in the SDK. The fact that they currently end up in dist/bin is a little unfortunate.

Comment 8

[Mike Hommey [:glandium]]

Well, they are in the SDK because of these rules
http://lxr.mozilla.org/seamonkey/source/config/Makefile.in#104

So, on line 105, $(HEADERS) should be replaced by $(DEPTH)/mozilla-config.h and line 135 should be removed. If you are okay with this I can send a patch for it.

Comment 9

[Benjamin Smedberg]

That sounds correct.

Comment 10

[Mike Hommey [:glandium]]

Attachment: patch

New patch doing what I said in comment #8

Comment 11

[Benjamin Smedberg]

Comment on attachment 256004 [details] [diff] [review]
patch

Does this work? Right now I think we have internal code that requires nsBuildID.h and nsStaticComponents.h to be in dist/include, I thought.

Comment 12

[Mike Hommey [:glandium]]

Not tested yet, I didn't have time to try it yet, and won't have for some time, sorry...

Comment 13

[Benjamin Smedberg]

Comment on attachment 256004 [details] [diff] [review]
patch

I have committed the second hunk of this patch (just the install:: target change).

Comment 14

[Mike Hommey [:glandium]]

I did test, and you're right, some internal code requires these files. Adding -I $(topsrcdir)/config in LOCAL_INCLUDES in the appropriate Makefile.in make it useless to have the files in dist/include.
For a xulrunner 1.8 build, this change is necessary in xpcom/build and xulrunner/app, but looking up for includes of these files in lxr.mozilla.org does  show which directories are impacted.
Do you think this update be done ?

Comment 15

[Benjamin Smedberg]

Let's not worry about it for the moment, and I'll get to it when we do the work to clear dist/include of really private headers. Marking FIXED because I checked in the install:: parts of this patch.