[Home]

Summary:ASTERISK-14351: [patch] Repeatedly building the asterisk directory repeatedly downloads the sounds files
Reporter:Philip Prindeville (pprindeville)Labels:
Date Opened:2009-06-22 00:18:26Date Closed:2011-01-08 19:34:00.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/BuildSystem
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) asterisk-1.6-bugid15370#2.patch
( 1) asterisk-1.6-bugid15370#7.patch
( 2) asterisk-bugid15370-1.6#6.patch
( 3) asterisk-trunk-bugid15370.patch
( 4) pprindeville_15370_combined.patch
Description:If you have a source tree, and repeatedly do "make ; make dist-clean" ... (lather, rinse, repeat) for multiple platforms, then you end up fetching the sounds files over and over again.

This fix allows one to cache the files outside of the source tree by setting the variable "SOUNDS_CACHE_DIR" to some path outside the tree.

Subsequent builds will copy the file from there when rebuilding.


****** ADDITIONAL INFORMATION ******

This has the added bonus of reducing unnecessary traffic to the Asterisk repository's server.
Comments:By: Philip Prindeville (pprindeville) 2009-06-22 00:25:53

Tested locally with and without caching enabled.

I've not tried this for languages other than English.

The sequence:

      if test ! -f $${PACKAGE}; then $(DOWNLOAD) $(WGET_ARGS) $(SOUNDS_URL)/$${PACKAGE}; fi; \
      if test ! -f $${PACKAGE}; then exit 1; fi; \

Was shortened to:

      if test ! -f $${PACKAGE}; then \
        ($(DOWNLOAD) $(WGET_ARGS) $(SOUNDS_URL)/$${PACKAGE} || exit 1); \
      fi; \

Hopefully "wget" fails with the appropriate status code in all cases.
By: Philip Prindeville (pprindeville) 2009-07-04 01:24:51

Updated with fix for 1.6.1.0.
By: Tilghman Lesher (tilghman) 2009-07-21 17:34:48

Should the definition for SOUNDS_CACHE_DIR be a "?=" define, instead, since the sounds directory is invoked from another Makefile?  It would be logical to be able to pass the directory name on the command line (or via an include) rather than requiring the variable to be set in the sounds Makefile itself.  Also, would it make sense to allow that directory name to be set from the configure script itself?

Additionally, in the macro definitions, I'd prefer that "sound" and "download" be spelled out.
By: Philip Prindeville (pprindeville) 2009-07-22 17:55:45

Amended as per comments above.
By: Philip Prindeville (pprindeville) 2009-08-14 16:36:55

Fixed reversion, and addressed comments from tilghman.

Added sha1sum checking as per chat.
By: Philip Prindeville (pprindeville) 2009-09-28 17:58:14

Anything else needed from me to move this along?
By: Philip Prindeville (pprindeville) 2009-11-23 14:20:26.000-0600

Can we get this into trunk for testing?
By: Tilghman Lesher (tilghman) 2009-11-23 15:27:35.000-0600

We don't commit things to trunk to test them.
By: Philip Prindeville (pprindeville) 2010-01-13 19:53:16.000-0600

Well, we've been using this fix in production builds for 4 months.

If there's an issue with it, we haven't encountered it.
By: Philip Prindeville (pprindeville) 2010-03-06 18:58:16.000-0600

Is there anything else we can provide at this time to assist with this getting closure?

We've submitted a fix on 2009-08-14.
By: Walter Doekes (wdoekes) 2010-03-09 02:01:22.000-0600

The idea of not having to download the audio files again and again sounds appealing to me. That would be an alternate fix to my qualm of the wontfix-closed https://issues.asterisk.org/view.php?id=16807 .
By: Paul Belanger (pabelanger) 2010-04-30 11:12:56

Am I missing something? I applied the patch:

./configure
make install
make dist-clean

./configure
make install

Sound files are download again.
By: Philip Prindeville (pprindeville) 2010-04-30 11:23:14

Aye, you did miss something:

./configure
mkdir sc
make install SOUNDS_CACHE_DIR=`pwd`/sc

...
By: Paul Belanger (pabelanger) 2010-04-30 11:41:00

Few updates / suggestions.
- Create ./configure option to disable/enable caching for sounds.
- Default SOUNDS_CACHE_DIR=cache with the ability to override.
- Is copying the .tar.gz from the CACHE_DIR back to sounds really required?  Simply install sounds directly from CACHE_DIR (trivial change to save HDD space).
- Enable build_tools/prep_tarball to use this logic (more to help with people building software packages [IE: dpkg])
By: Philip Prindeville (pprindeville) 2010-04-30 11:46:30

Just made SOUNDS_CACHE_DIR an option in the second (optional) patch.

If, after the fix gets committed, you want to optimize it to optionally use hardlinks instead of copied, please do so.  But at this point the fix has been backburnered long enough.  I don't want to further delay it with a "nice-to-have" functionality.
By: Philip Prindeville (pprindeville) 2010-04-30 11:47:26

To be clear, with the second patch, you do:

mkdir sc
./configure ... SOUNDS_CACHE_DIR=`pwd`/sc

...
By: Leif Madsen (lmadsen) 2010-05-03 14:12:31

Sent back to Confirmed because more work is necessary here.
By: Philip Prindeville (pprindeville) 2010-05-03 14:20:21

What work would that be?
By: Philip Prindeville (pprindeville) 2010-05-03 15:37:36

This fix represents a significant change to the makefiles by way of introducing a layer of macro-abstraction.

It's a fair amount of complexity.  It's a rewrite of about 50% of the makefile.

Adding a second, unrelated behavior to how things operate would make this change potentially much more destabilizing.

Also, once the sounds cache has been built, and the files have been copied out, there's (a) no risk of two build processes corrupting a shared file (because there are no writeable shared files), and (b) absolutely no issue with cloning/moving a project's root directory (including to another filesystem) because we don't use symlinks or hardlinks.
By: Philip Prindeville (pprindeville) 2010-05-03 16:29:08

FYI: the patches attached were made against 1.6.2, but apply to trunk just fine.
By: Philip Prindeville (pprindeville) 2010-05-03 16:43:10

BTW: If you are trying the second patch, after applying it, you need to do:

rm configure
./bootstrap.sh

to rebuild the configure target from the configure.ac file with the patch in effect.
By: Sean Bright (seanbright) 2010-05-04 14:14:36

I created a combined patch that applies cleanly to current trunk.
By: Philip Prindeville (pprindeville) 2010-05-17 14:48:37

Please advise what's required to get closure on this issue.
By: Tilghman Lesher (tilghman) 2010-05-17 16:24:45

Neither of the two latest patches apply cleanly to trunk.  You'll need to update the patches again.
By: Sean Bright (seanbright) 2010-05-17 16:59:40

pprindeville_15370_combined.patch now applies cleanly against trunk.
By: Digium Subversion (svnbot) 2010-05-17 18:49:16

Repository: asterisk
Revision: 263724

U   trunk/autoconf/ast_ext_lib.m4
U   trunk/configure
U   trunk/configure.ac
U   trunk/include/asterisk/autoconfig.h.in
U   trunk/makeopts.in
U   trunk/sounds/Makefile

------------------------------------------------------------------------
r263724 | tilghman | 2010-05-17 18:49:15 -0500 (Mon, 17 May 2010) | 8 lines

Cache sound tarfiles in a common directory, such that a clean reinstall does not force a re-download of the tarballs.

(closes issue ASTERISK-14351)
Reported by: pprindeville
Patches:
      asterisk-trunk-bugid15370.patch uploaded by pprindeville (license 347)
Tested by: pprindeville, tilghman, seanbright

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=263724
By: Digium Subversion (svnbot) 2010-06-03 20:41:25

Repository: asterisk
Revision: 267820

_U  branches/1.6.2/
U   branches/1.6.2/autoconf/ast_ext_lib.m4
U   branches/1.6.2/configure
U   branches/1.6.2/configure.ac
U   branches/1.6.2/makeopts.in
U   branches/1.6.2/sounds/Makefile

------------------------------------------------------------------------
r267820 | tilghman | 2010-06-03 20:41:25 -0500 (Thu, 03 Jun 2010) | 19 lines

Merged revisions 263724,267819 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r263724 | tilghman | 2010-05-17 18:49:15 -0500 (Mon, 17 May 2010) | 8 lines
 
 Cache sound tarfiles in a common directory, such that a clean reinstall does not force a re-download of the tarballs.
 
 (closes issue ASTERISK-14351)
  Reported by: pprindeville
  Patches:
        asterisk-trunk-bugid15370.patch uploaded by pprindeville (license 347)
  Tested by: pprindeville, tilghman, seanbright
........
 r267819 | tilghman | 2010-06-03 20:36:46 -0500 (Thu, 03 Jun 2010) | 2 lines
 
 If there's a default, turn it on, even when the option isn't specified.
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=267820
By: Philip Prindeville (pprindeville) 2010-06-03 21:42:15

I'm looking at the 1.6.2 commit into sounds/Makefile, line 172:

install: ${SOUNDS_CACHE_DIR}  $(SOUNDS_DIR)/en $(SOUNDS_DIR)/es $(SOUNDS_DIR)/fr $(MOH_DIR) $(CORE_SOUND_TAGS) $(EXTRA_SOUND_TAGS) $(MOH_TAGS)


Is that supposed to be ${...} and not $(...) for SOUNDS_CACHE_DIR?

By: Jason Parker (jparker) 2010-07-27 13:47:04

this code has changed, and your comment isn't relevant anymore