MantisBT - CMake
View Issue Details
0007000CMakeCPackpublic2008-05-10 07:302010-09-09 23:48
Eric NOULARD 
Eric NOULARD 
normalminoralways
closedfixed 
CMake-2-6 
CMake 2.8.3CMake 2.8.3 
0007000: CPack RPM generator should always "CPACK_SET_DESTDIR"
I think it has already been discussed on the list:
http://www.cmake.org/pipermail/cmake/2008-March/020685.html [^]
http://www.cmake.org/pipermail/cmake/2008-March/020690.html [^]
http://www.cmake.org/pipermail/cmake/2008-April/020833.html [^]

And people encounter the problem again with CPack RPM:
http://www.cmake.org/pipermail/cmake/2008-May/021506.html [^]

I would say that CPACK_SET_DESTDIR to ON should
be the default for CPackRPM.

With the attached testcase you get:

$ make package
Run CPack packaging tool...
CPack: Create package using RPM
CPack: Install projects
CPack: - Run preinstall target for: myscript
CPack: - Install project: myscript
CMake Error at cmake_install.cmake:40 (FILE):
  file cannot create directory: /opt/sbin. Maybe need administrative
  privileges.


CPack Error: Error when generating package: myscript
make: *** [package] Erreur 1

Simply because CPACK_SET_DESTDIR wasn't set to ON in CMakeLists.txt
No tags attached.
related to 0007410closed Eric NOULARD CPack SET_DESTDIR feature is broken by Components installation 
related to 0010701closed Eric NOULARD Lack of "%doc, %config, Obsolete" in CPackRPM 
related to 0010339closed Eric NOULARD RPM generator does not understand path in destination 
child of 0010935closed Eric NOULARD Lack of support for relocatable package 
bz2 myscript-1.0-src.tar.bz2 (712) 2008-05-10 07:30
https://public.kitware.com/Bug/file/1452/myscript-1.0-src.tar.bz2
patch CPackRPM-SET_DESTDIR_On.patch (624) 2008-05-10 07:39
https://public.kitware.com/Bug/file/1453/CPackRPM-SET_DESTDIR_On.patch
patch cpack_destdir_warn.patch (1,820) 2008-09-04 18:01
https://public.kitware.com/Bug/file/1704/cpack_destdir_warn.patch
patch cpack_destdir_warn-2.patch (4,952) 2008-09-06 19:25
https://public.kitware.com/Bug/file/1708/cpack_destdir_warn-2.patch
patch 0001-Authorize-warning-or-error-for-absolute-path-destina.patch (3,255) 2010-07-04 12:40
https://public.kitware.com/Bug/file/3222/0001-Authorize-warning-or-error-for-absolute-path-destina.patch
patch 0001-CPack-Backward-compatibly-enforce-DESTDIR-for-DEB-an.patch (5,468) 2010-08-23 11:48
https://public.kitware.com/Bug/file/3337/0001-CPack-Backward-compatibly-enforce-DESTDIR-for-DEB-an.patch
patch 0002-CPack-Enable-better-handling-of-absolute-installed-f.patch (7,938) 2010-08-23 11:48
https://public.kitware.com/Bug/file/3338/0002-CPack-Enable-better-handling-of-absolute-installed-f.patch
Issue History
2008-05-10 07:30Eric NOULARDNew Issue
2008-05-10 07:30Eric NOULARDFile Added: myscript-1.0-src.tar.bz2
2008-05-10 07:39Eric NOULARDNote Added: 0011788
2008-05-10 07:39Eric NOULARDFile Added: CPackRPM-SET_DESTDIR_On.patch
2008-05-10 17:44Alex NeundorfStatusnew => assigned
2008-05-10 17:44Alex NeundorfAssigned To => Alex Neundorf
2008-05-11 05:26Alex NeundorfNote Added: 0011805
2008-05-11 14:40Eric NOULARDNote Added: 0011809
2008-05-12 16:22Eric NOULARDNote Added: 0011829
2008-05-12 16:40Alex NeundorfNote Added: 0011830
2008-05-12 17:25Eric NOULARDNote Added: 0011836
2008-07-29 09:30YuriNote Added: 0012853
2008-08-25 04:33Eric NOULARDNote Added: 0013166
2008-09-04 15:54Alex NeundorfNote Added: 0013299
2008-09-04 17:58Eric NOULARDNote Added: 0013301
2008-09-04 18:01Eric NOULARDFile Added: cpack_destdir_warn.patch
2008-09-05 10:57Brad KingRelationship addedrelated to 0007410
2008-09-05 10:59Brad KingAssigned ToAlex Neundorf => David Cole
2008-09-05 11:12Brad KingNote Added: 0013303
2008-09-05 11:15David ColeNote Added: 0013304
2008-09-06 14:35Eric NOULARDNote Added: 0013333
2008-09-06 19:25Eric NOULARDNote Added: 0013335
2008-09-06 19:25Eric NOULARDFile Added: cpack_destdir_warn-2.patch
2009-09-06 04:29Alex NeundorfNote Added: 0017321
2010-03-01 16:51Alex NeundorfNote Added: 0019671
2010-03-02 02:41Eric NOULARDNote Added: 0019682
2010-03-02 14:31Alex NeundorfNote Added: 0019688
2010-03-02 16:45Eric NOULARDNote Added: 0019689
2010-03-02 17:14Alex NeundorfNote Added: 0019690
2010-03-02 17:23Eric NOULARDNote Added: 0019692
2010-07-04 10:52Eric NOULARDRelationship addedrelated to 0010701
2010-07-04 11:30Eric NOULARDRelationship addedchild of 0010935
2010-07-04 12:40Eric NOULARDNote Added: 0021233
2010-07-04 12:40Eric NOULARDFile Added: 0001-Authorize-warning-or-error-for-absolute-path-destina.patch
2010-07-06 02:38Eric NOULARDRelationship addedrelated to 0010339
2010-08-07 09:21chriswolfNote Added: 0021690
2010-08-07 09:55chriswolfNote Edited: 0021690
2010-08-07 10:17chriswolfNote Edited: 0021690
2010-08-07 10:23chriswolfNote Edited: 0021690
2010-08-07 10:24chriswolfNote Edited: 0021690
2010-08-17 23:42David ColeNote Added: 0021787
2010-08-17 23:42David ColeAssigned ToDavid Cole => Eric NOULARD
2010-08-22 16:10Eric NOULARDNote Added: 0021883
2010-08-23 11:48Eric NOULARDNote Added: 0021895
2010-08-23 11:48Eric NOULARDFile Added: 0001-CPack-Backward-compatibly-enforce-DESTDIR-for-DEB-an.patch
2010-08-23 11:48Eric NOULARDFile Added: 0002-CPack-Enable-better-handling-of-absolute-installed-f.patch
2010-08-23 12:06Eric NOULARDNote Added: 0021896
2010-08-24 16:36Eric NOULARDNote Added: 0021932
2010-08-31 16:34Eric NOULARDNote Added: 0022034
2010-08-31 16:34Eric NOULARDStatusassigned => closed
2010-08-31 16:34Eric NOULARDResolutionopen => fixed
2010-08-31 16:34Eric NOULARDFixed in Version => CMake-2-8
2010-08-31 18:03David ColeTarget Version => CMake 2.8.3
2010-09-09 23:48David ColeFixed in VersionCMake-2-8 => CMake 2.8.3

Notes
(0011788)
Eric NOULARD   
2008-05-10 07:39   
The following patch enforce DESTDIR to ON for CPackRPM.
I do not see a case for which this would harm (at least for RPM case).
This does not seems to have side-effect on other generator
(DEB generator still have an error for absolute path).
(0011805)
Alex Neundorf   
2008-05-11 05:26   
+ this->SetOption("CPACK_SET_DESTDIR", "ON");

I guess the same should be done for the deb generator ?

Alex
(0011809)
Eric NOULARD   
2008-05-11 14:40   
Yes I think so, may be you can ask to Mathieu, for sure.

Personnally,
I cannot figure out why _ANY_ generator would not benefit
from this change in order to be able to handle absolute path
but there must be a reason I did not see?
(0011829)
Eric NOULARD   
2008-05-12 16:22   
I've just checked that on my Linux system with CMake 2.6.0

DEB
TGZ
ZIP
NSIS
TBZ2
...

all suffers from the
"file cannot create directory: /opt/sbin. Maybe need administrative
  privileges."

They don't if I
SET(CPACK_SET_DESTDIR "ON")
in my CMakeList.txt

I think they share the fact that CPack calls something like "make install"
for which absolute install path fail.

So the question is:
Why CPack does not _ALWAYS_ use DESTDIR?
At least for UNIX platform?
(0011830)
Alex Neundorf   
2008-05-12 16:40   
Can you please bring this up on the cmake list ?
What would you think about cvs write access for maintaining the RPM generator ? (not that I would have anything to decide on this...)

Alex
(0011836)
Eric NOULARD   
2008-05-12 17:25   
Done I did raise the issue on the ML.
I would be OK (and honored :-) with cvs write access on RPM generator
for maintainance.

I may need some recommandation on CMake cvs commit policy.
(0012853)
Yuri   
2008-07-29 09:30   
Checking for IsSet("CPACK_SET_DESTDIR") and reporting error would be less surprise for regular user.
(0013166)
Eric NOULARD   
2008-08-25 04:33   
I did propose to add warning in a patch attached to
http://public.kitware.com/Bug/view.php?id=7435 [^]
(0013299)
Alex Neundorf   
2008-09-04 15:54   
It would be cool if you could try to write a patch which generates the warning for all package generators.
Can cmake -P cmake_install.cmake detect whether it is executed by cpack and print the warning just in this case ?
If cpack executes cmake_install.cmake directly, it should be possible to give any additional variables to it if required (e.g. something like -DCMAKE_CALLED_BY_CPACK=TRUE)

Alex
(0013301)
Eric NOULARD   
2008-09-04 17:58   
I do not get your point?
If we want to warn for ALL generators then you may simply add the warning
inside
Source/CPack/cmCPackGenerator.cxx: cmCPackGenerator::InstallProjectViaInstallCMakeProjects

Currently we have:

if (!setDestDir)
          {
          tempInstallDirectory += this->GetPackagingInstallPrefix();
          }

if ( setDestDir )
          {
           ...
          }

I may add a warning in the first case.

It seems easier to me than guessing from within
cmake_install.cmake whether if we were called by CPack or not.
Is this OK for you?
However is it acceptable to warn for ALL CPack generators?
Are there only non-cmake project that use CPack with absolute installed file
"on purpose"?

What would be better is to detect if there IS effectively
an absolutely installed file in the project, because if there is no
absolutely installed file there nothing to warn about
(which may be the more general case).
 
I did have a look inside the cmake_install.cmake files
and it seems that in this file it is too late here to guess
whether if there is an absolutely installed file.
"each install generator to write its code.".

seen in Source/cmLocalGenerator.cxx:cmLocalGenerator::GenerateInstallRules

thus I end up in
Source/cmInstallGenerator.cxx:cmInstallGenerator::AddInstallRule
where we can check if the DESTINATION arg is an absolute file name
or not.

You'll find patch attached which do both checking and issue warning
for all CPack generators if DESTDIR is not set.
You get additionnal ERROR if you effectively use absolute path.
(0013303)
Brad King   
2008-09-05 11:12   
Dave Cole and I just talked about this issue a bit. A key point is that there are two kinds of CPack generators:

  (A) Those creating packages with absolute install locations (DEB, RPM)
  (B) Those creating packages with relocatable trees (ZIP, STGZ, NSIS)

Usually TZ, and TGZ are type (B) but they can be used for type (A) too.

For type (A), DESTDIR should always be used.
For type (B), DESTDIR should never be used (nor any absolute install locations).

This distinction needs to be made explicit in CPack. Instead of setting an option for DESTDIR, each generator should use what makes sense for its type. We could provide an option to set whether the project belongs to type (A) or type (B), and then only the matching generator types would be allowed.

We can use the policy mechanism (http://www.cmake.org/Wiki/CMake_Policies [^]) to change behavior while preserving compatibility with existing projects.
(0013304)
David Cole   
2008-09-05 11:15   
We should also add code that detects if any absolute locations are used in install rules, and insist on use of a type (A) CPack generator if there are any.
(0013333)
Eric NOULARD   
2008-09-06 14:35   
To Brad:
   I understand the difference you make between A and B, it is a good
   idea but I don't think that you may say that a _PROJECT_
   is of type A or B.
   I use CMake for 2 projects which need both
           * RPM/DEB generator on Linux
   __AND__ * NSIS + ZIP on windows.
   So I think we cannot say the PROJECT is of type A or B
   or I will do something odd like
   IF(WIN32)
     SET(CMAKE_PROJECT_TYPE B)
   ELSE(WIN32)
     SET(CMAKE_PROJECT_TYPE A)
   ENDIF(WIN32)
   which seems awkward?

   However I think we may force DESTDIR on a CPack generator basis,
   and as

David told:
   "We should also add code that detects if any absolute locations are used in install rules"
    I totally agree with this, and that's inside
    my "cpack_destdir_warn.patch".

So my opinion is:

   1) Each CPack generator should tell if he is enforcing DESTDIR
      or not.
   2) We should monitor if "any absolute locations are used in install rules"

   3) We should generically throw an error if any "absolute location" are used
      with a CPack generator not enforcing DESTDIR.

In order to summarize, type A or type B project should be "detected"
by monitoring install rules destination. Project is type A if he use
at least one absolute install locations.

Each CPack generator should tell if is of type A or type B.

Then CPack generator should throw an error if a type B cpack generator
is used with a type A project.
Note that a type B project may well be used with a type A CPack generator,
I expect that most project should be of type B and should be able
to use _ANY_ cpack generator (as of today).

I'll try to provide a patch later tonight (UTC+2) or tomorrow.
(0013335)
Eric NOULARD   
2008-09-06 19:25   
Here comes a patch which illustrates the previous idea.
I did not have time to look at how to use CMAKE Policies in order
to maintain compatibility.

With the current patch:
  - RPM and DEB generators enforces DESTDIR usage
  - other generator do not
    (cmCPackGenerator::ShouldEnforceDestdir returns false)

CMake project using CPack will make CPack fail if they use
absolutely installed path with generator who are not enforcing DESTDIR usage.
This is done through inserted IF + MESSAGE(FATAL_ERROR ...) command
in cmake_install.cmake (see cmInstallGenerator.cxx)

However if ones puts:
SET(CPACK_SET_DESTDIR "ON")
in its CMakeList.txt then every CPack generator will succeed
(as it is the case today)

Tell me what you think about this.
(0017321)
Alex Neundorf   
2009-09-06 04:29   
There can be also tgz packages which are not relocatable, e.g. all Slackware packages are simple tgz files but they always should be installed from / .
(and I think the same is true for other "simple" just-create-an-archive generators).

Also, it doesn't depend only on the install paths, there are also the RPATH settings to be considered. If somewhere RPATH is set to the install location of a library within the project, the package is not relocatable anymore.

Alex
(0019671)
Alex Neundorf   
2010-03-01 16:51   
I used the rpm generator today.
I had CMAKE_INSTALL_PREFIX set to /opt/Foo, but the generated rpm had all the stuff in /usr, e.g. /usr/bin/myfoo instead of /opt/Foo/bin/myfoo.

How did that happen ?

Setting CPACK_USE_DESTDIR to ON fixed the issue.

I think something should be done on this.

Alex
(0019682)
Eric NOULARD   
2010-03-02 02:41   
I'll have a look at this.
If we follow recommandation from Brad I think we can enforce DESTDIR
for both RPM and DEB.
(0019688)
Alex Neundorf   
2010-03-02 14:31   
Do you know why without DESTDIR the resulting rpm contained everything in /usr (and not /usr/local) ?

Also, with DESTDIR, the created tar.gz file had a strange layout, the contents where then in Foo-1.2.3/opt/bin/.
So for rpm DESTDIR needs to be on, for tgz it either needs to be off, or if it is set that prefix directory must not be prepended.

Alex
(0019689)
Eric NOULARD   
2010-03-02 16:45   
I think I know where the "/usr" comes from:

int cmCPackRPMGenerator::InitializeInternal()
{
  this->SetOptionIfNotSet("CPACK_PACKAGING_INSTALL_PREFIX", "/usr");

  return this->Superclass::InitializeInternal();
}

This have been added by commit a04647f6ee5b1ce37d690802e11d53ce5dab6239
Author: Alexander Neundorf <neundorf@kde.org>
Date: Fri Aug 17 13:33:29 2007 +0000

    STYLE: InitializeInternal() is unused
    
    Alex

you :-)

So you should be able to overwrite this default using
CPACK_PACKAGING_INSTALL_PREFIX.


Regarding why the default value is not CMAKE_INSTALL_PREFIX...
I don't know.
(0019690)
Alex Neundorf   
2010-03-02 17:14   
Hmm, maybe I committed this on behalf of the actual author, which wasn't me.
Was it you ? ;-)

Ok, anyway.
I just checked, the cygwin binary, deb and packagemaker generators do the same.

I have no idea why.

Alex
(0019692)
Eric NOULARD   
2010-03-02 17:23   
I do not remember of doing such addition.
Looks like some "global" clean-up of CPack generators.
That said being able to have both
CPACK_PACKAGING_INSTALL_PREFIX
and
CMAKE_INSTALL_PREFIX is a good feature.

Now the default to "/usr" seems there since a long time ago
ChangeLog.txt:
2007-10-31 08:50 david.cole
          ENH: Add
          variable CPACK_PACKAGING_INSTALL_PREFIX to allow overriding the
          CPack GetPackagingInstallPrefix from a project's CMakeLists file
          if necessary. Could be used to remove the annoying /usr prefix
          still used by default in the Mac PackageMaker generator.
   
We could change the default to be CMAKE_INSTALL_PREFIX
but it would possibly break current usage.
(0021233)
Eric NOULARD   
2010-07-04 12:40   
Ok let's give this another try.
The freshly attached patch will add WARNING or ERROR into generated install scripts. Those ERROR/WARNING may be set if
CPACK_WARN_ABSOLUTE_DESTINATION or
CPACK_ERROR_ABSOLUTE_DESTINATION are set by the CPack generator
(or the command line: cpack -D CPACK_WARN_ABSOLUTE_DESTINATION=1 )

This way on can easily find the concerned file/target of the absolute destination.

Moreover I think that since we want to be backward compatible we must
find a way to transmit informations from
CMake time: when we know there is absolute DESTINATION
to
CPack time: when we shall take a decision.

Currently there is only warnings at install time, but it's better than nothing.
Moreover the CPACK_xxxx_ABSOLUTE_DESTINATION are use iff setDestdir is OFF
because when DESTDIR is on absolute destination should not be a problem.

I'd rather add a CPACK_PACKAGE_HAS_ABSOLUTE_DESTINATION in the generated
CPackConfig.cmake, but due to the recursive nature of the install commands
we do not have this information when CPackConfig.cmake is generated :-(

However if we could (don't know how yet)
and since this file will be loaded by any CPack generator
the generator may decide what to do:
enforce DESTDIR, WARN globally, WARN locally from install files etc...

What do you think
(0021690)
chriswolf   
2010-08-07 09:21   
(edited on: 2010-08-07 10:24)
I propose implementing a per-generator override of the absolute install path.

Currently, the default absolute install path can be overridden by specifying
CPACK_PACKAGING_INSTALL_PREFIX either in the CMake/CPack file or on the command line. Unfortunately, this is all-or-nothing. It would be convenient to be able to override the default path on a per-generator basis, as discussed here:

http://www.mail-archive.com/cmake@cmake.org/msg16180.html [^]

We could introduce new per-generator variables of the form:

CPACK_<GENNAME>_PACKAGING_INSTALL_PREFIX

where "<GENNAME>" is DEB, RPM, etc.

This could be quickly and simply implemented by overriding the base class
method, cmCPackGenerator::GetPackagingInstallPrefix() in one or more
per-generator specialization class implementations. Here is an example for
the DEB generator:

const char* cmCPackDebGenerator::GetPackagingInstallPrefix()
{

  const char* def = this->GetOption("CPACK_DEB_PACKAGING_INSTALL_PREFIX");
  if ( def && *def )
    {
    cmCPackLogger(cmCPackLog::LOG_DEBUG, "GetPackagingInstallPrefix: '"
        << def << "' via CPACK_DEB_PACKAGING_INSTALL_PREFIX" << std::endl);
    return def;
    }

  // fall-through - check CPACK_PACKAGING_INSTALL_PREFIX
  return this->cmCPackGenerator::GetPackagingInstallPrefix();
}

(0021787)
David Cole   
2010-08-17 23:42   
Eric,

I'm comfortable with you making the call on this issue. If you think you can fix it in a reasonable manner that is backwards compatible, then please feel free to do so. I'll be happy to review any changes you push to 'next'. Therefore, I'm assigning this issue to you...

Thanks for your efforts on the CPack front lately.
(0021883)
Eric NOULARD   
2010-08-22 16:10   
Hi David,

Thank you for your trust.
I'll try something backward compatible beginning with DEB and RPM.
RPM would may be the better test case because I know it well
and one should be able to build a relocatable RPM even if
some files have to be installed with absolute path like /etc/xxx.

I may tackle the DESTDIR problem for ArchiveGenerator afterwards.

I will let you know when I have pushed something interesting to next
may during this week.
(0021895)
Eric NOULARD   
2010-08-23 11:48   
Hi David,

I have just pushed my proposal to github,
would you be able to pull from there or would you like
me to push on next?

http://github.com/TheErk/CMake/tree/CPack-FixDESTDIR-Issue7000 [^]

I will upload corresponding patches in the tracker and post another comment in order to explain my solution proposal, which is "as-much-backward-compatible-as-it-could-be",
(0021896)
Eric NOULARD   
2010-08-23 12:06   
Explanation of the first patch:

  The idea of the patch is to enforce CPACK_SET_DESTDIR to I_ON which means "Internally switched ON". Using this I can maintain backward compatibility for those who are already using set(CPACK_SET_DESTDIR "ON").

  So RPM and DEB now set(CPACK_SET_DESTDIR "I_ON") if CPACK_SET_DESTDIR was OFF
(default value given by CPack.cmake). If CPACK_SET_DESTDIR is I_ON we override
CPACK_INSTALL_PREFIX with the value CPACK_PACKAGING_INSTALL_PREFIX (instead of
taking the default CMAKE_INSTALL_PREFIX enforce by CPack.cmake). This is done
to mimic the current behavior when DESTDIR was OFF.

After that all is running well for absolute files because we used DESTDIR
"behind the scene".

Concerning the second patch:

  Now that we can automagically handle absolute destination files, we must
do more in order to be able to e.g. build a relocatable RPM package.
The idea is that usually "absolute destination files" are config files
(in /etc/xxx or any subdir of this). So if the specific generator did know
about those "absolute dest files" may be he can flag them as "%config".

  The problem is CPack doesn't know anything about "absolute file" because
installation are handled by CMake script generated by InstallGenerator
** at CMake time **. The proposed solution is to defined a new var
"CPACK_ABSOLUTE_DESTINATION_FILES" in each installation script such that
CPack generic generator may collect the "absolute files" during install and
define the same var "CPACK_ABSOLUTE_DESTINATION_FILES" before calling
the specific generator PackageFiles() method.

   CPackRPM is catching this var in order to generate a relocatable RPM
even if some absolutely installed files have been given.

All thoses changes SHOULD NOT affect other CPack Generators.
The changes should be fully backward compatible.
The main difference is that now project that do install absolute file
will get the expected RPM/DEB.

The only "trouble" will be for people who are mistakenly using absolute
destinations, they will now get an RPM with may-be unwanted absolute paths.
We can further improve that by specifying some "ABSOLUTE_FILE_ACCEPTED_REGEX"
the generator may check against the list CPACK_ABSOLUTE_DESTINATION_FILES.

The same idea may be applied to archive generators but I'll wait to merge
the component thing first., moreover RPM and DEB were the most urgent I think.
(0021932)
Eric NOULARD   
2010-08-24 16:36   
The patch proposal has been pushed to next.
(0022034)
Eric NOULARD   
2010-08-31 16:34   
Merged to master:

commit f95074ba7025aadf2268dae910184c28be797afc
Merge: 2391002 3178767
Author: Brad King <brad.king@kitware.com>
Date: Tue Aug 31 14:22:59 2010 -0400

    Merge topic 'CPack-FixDESTDIR-Issue7000'
    
    3178767 Merge 'CPack-FixDESTDIR-Issue7000' from github.com:TheErk/CMake
    6a521f8 CPack Enable better handling of absolute installed files
    40dc97d CPack Backward-compatibly enforce DESTDIR for DEB and RPM