[PATCH] Fix test-release.sh to use DESTDIR

[Dan Liew]

Hi,

On 1 July 2015 at 07:37, Hans Wennborg <hans at chromium.org> wrote:
> Hi Dan,
>
> Sorry for the slow reply. I'm on vacation and just checking in on work
> mail occasionally.

No problem.

> On Fri, Jun 26, 2015 at 6:22 PM, Dan Liew <dan at su-root.co.uk> wrote:
>> This patch tries to fix the test-release.sh script which looks like
>> it's used for creating the final release binaries.
>
> Yes, that's the script for creating the release binaries.
>
>> The problem is that --prefix was set to a temporary build directory
>> which then shows up in the final binaries and build files. In
>> particular the installed LLVMConfig.cmake and LLVMExports.cmake files
>> contain these paths making them completely broken.
>>
>> You can observe this if you take a look at the contents of the binary
>> tarballs (Ubuntu tarballs) on LLVM's release page for LLVM 3.5.
>>
>> For the LLVM 3.6.x tarballs the CMake files are completely missing
>> (Ubuntu tarballs). I don't understand why, the test-release.sh script
>> doesn't appear to do anything to remove these files explicitly. What
>> happened here?
>
> I'm not sure what changed between the 3.5 and 3.6 releases in this
> regard. I don't think the script itself has changed, but maybe the
> build system was changed to not generate the .cmake files in autoconf
> builds, as they would probably be broken.

Looking at the release_36 branch I can't see anything obvious that
disabled the generation of the CMake files.

>> The way this is normally handled is that --prefix is set to where it
>> is intended for the files to be installed and DESTDIR is set when
>> running ``make install`` to have files be installed elsewhere in
>> preparation for tarballing.
>>
>> This patch sets ``--prefix`` to ``/usr/local`` by default (although is
>> can be changed using a command line option to the script) and uses
>> DESTDIR to install the files elsewhere.
>>
>> ``/usr/local`` seems like a sensible default to me that should avoid
>> where Linux package managers typically install their packages and I
>> think this is also used on OSX too.
>
> The problem is that the package is built by doing "make install" on
> the Phase 3 build, and then creating a tarball from that directory. So
> when running the script, the install dir really needs to be a
> temporary directory.

Yes I understand that but --prefix by convention should always be set
to where files will eventually be installed because
the binaries may contain code that depends on this (e.g. the
llvm-config.h file has these paths in it). The location to install
files for the purpose of tarballing should be set
via the ``DESTDIR`` makefile variable.

Essentially when ``make install`` is run things are installed into

$(DESTDIR)$(PROJ_prefix)

by default DESTDIR is empty when so running ``make install`` will
install files into the $(PROJ_prefix). $(PROJ_prefix) is the value
passed to the ``--prefix`` argument to the configure script (typically
it's /usr/)

If you ran ``make install DESTDIR=/home/me/temp_build/`` and
$(PROJ_prefix) was set to ``/usr/`` then files would be installed to

/home/me/temp_build/usr/

e.g. you'd find the llvm-as binary at
``/home/me/temp_build/usr/bin/llvm-as``, but the binaries will be
built assuming that they will be installed in ``/usr/``.

So

- $(PROJ_prefix) is the location that files are intended to be
installed. Code in a binary may depend on this and changing the value
will trigger a rebuild of the binaries
- $(DESTDIR) is an additional prefix which allows installed files to
be placed in a temporary directory to be tarballed (also know as
staged install). Setting this variable will not trigger a rebuild of
binaries.

Currently the test-release script does this wrong. The prefix is set
to the tarball directory which ends up in the binaries and generated
CMake files. The prefix should instead be set to where the files will
eventually be installed (i.e. when unpacked from the tarball -
typically /usr/local) and the DESTDIR makefile variable should be used
to place files in a temporary directory for creating the tarball.

Does this make sense? This concept is also explained (probably in a
much clearer way) at
https://www.gnu.org/prep/standards/html_node/DESTDIR.html

My patch tries to fix this. Although I haven't had time to test the
patch actually works.

> I think the fix we need would be to make the --prefix used when
> building not affect the binaries or the .cmake files. I'm hoping to
> switch to using cmake for the 3.7 release (see
> http://reviews.llvm.org/D10715). Do you think the CMAKE_INSTALL_PREFIX
> flag will create the same problem?

CMAKE_INSTALL_PREFIX is analogous to Autoconf's ``--prefix``. The
CMake generated makefiles support the ``DESTDIR`` makefile variable so
the CMake build should be treated in the same way

Thanks,
Dan.