Closed Bug 1043692 Opened 5 years ago Closed 4 years ago

move DIST_INSTALL into moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: froydnj, Assigned: glandium)

References

Details

Attachments

(2 files)

This is pretty easy; the most complex part is describing exactly what DIST_INSTALL does, which I think I understand now.
Actually, there's something even easier for, I guess, quite a few DIST_INSTALLs: remove them. (when looking at the patch in bug 1042878, i saw those in intl/unicharutil/util/*, and I bet they are useless nowadays ; I'm sure there are many others)
(and many others, like, I suspect, the ones under xpcom/*, could be replaced with NO_EXPAND_LIBS)
(In reply to Mike Hommey [:glandium] from comment #1)
> Actually, there's something even easier for, I guess, quite a few
> DIST_INSTALLs: remove them. (when looking at the patch in bug 1042878, i saw
> those in intl/unicharutil/util/*, and I bet they are useless nowadays ; I'm
> sure there are many others)

I'm not sure I'm prepared to have the conversation about what libraries we should and shouldn't export.  I guess Benjamin might have an opinion about ceasing to export librdfutil_external, libunicharutil_external, and maybe a few others?

(In reply to Mike Hommey [:glandium] from comment #2)
> (and many others, like, I suspect, the ones under xpcom/*, could be replaced
> with NO_EXPAND_LIBS)

NO_EXPAND_LIBS is somewhat less obvious in its effects in this case, IMHO, compared to DIST_INSTALL.  NO_EXPAND_LIBS seems like it should just be renamed, since the build/ cases of NO_EXPAND_LIBS should be replaced with DIST_INSTALL, and the js/src/ usage of NO_EXPAND_LIBS can be something more sensible.  WDYT?
Flags: needinfo?(mh+mozilla)
(In reply to Nathan Froyd (:froydnj) from comment #3)
> (In reply to Mike Hommey [:glandium] from comment #1)
> > Actually, there's something even easier for, I guess, quite a few
> > DIST_INSTALLs: remove them. (when looking at the patch in bug 1042878, i saw
> > those in intl/unicharutil/util/*, and I bet they are useless nowadays ; I'm
> > sure there are many others)
> 
> I'm not sure I'm prepared to have the conversation about what libraries we
> should and shouldn't export.  I guess Benjamin might have an opinion about
> ceasing to export librdfutil_external, libunicharutil_external, and maybe a
> few others?

Anything that's not in the SDK doesn't need to be in dist/bin|dist/lib anymore unless it ends up in the final product. The only reason we needed things in dist/lib was to simplify *LIBS things in the Make land.

> (In reply to Mike Hommey [:glandium] from comment #2)
> > (and many others, like, I suspect, the ones under xpcom/*, could be replaced
> > with NO_EXPAND_LIBS)
> 
> NO_EXPAND_LIBS is somewhat less obvious in its effects in this case, IMHO,
> compared to DIST_INSTALL.  NO_EXPAND_LIBS seems like it should just be
> renamed, since the build/ cases of NO_EXPAND_LIBS should be replaced with
> DIST_INSTALL, and the js/src/ usage of NO_EXPAND_LIBS can be something more
> sensible.  WDYT?

There is no case of NO_EXPAND_LIBS that should be replaced with DIST_INSTALL. They do different things. DIST_INSTALL tells to copy stuff in dist/bin or dist/lib. It has the side effect of kind of implying NO_EXPAND_LIBS. NO_EXPAND_LIBS tells to create a real library file instead of a fake lib used by expandlib (a .desc file ; although in that case, the .desc file is still created). Yes, it could be renamed.
Flags: needinfo?(mh+mozilla)
As I thought I needed this knowledge for some other reason, I just went ahead. Turns out I don't need it, but now that it's done...
Assignee: nfroyd → mh+mozilla
Attachment #8603635 - Flags: review?(gps)
Attachment #8603636 - Flags: review?(gps)
Depends on: 1162779
Comment on attachment 8603635 [details] [diff] [review]
Add a DIST_INSTALL variable to moz.build, and replace NO_DIST_INSTALL with it

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

Mostly a semantic refactor. LGTM.
Attachment #8603635 - Flags: review?(gps) → review+
Comment on attachment 8603636 [details] [diff] [review]
Move DIST_INSTALL to moz.build

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

I love reviewing patches that remove Makefile.in files!
Attachment #8603636 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/59e335c30dfd
https://hg.mozilla.org/mozilla-central/rev/9bc266372d00
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.