View Issue Details [ Jump to Notes ] | [ Print ] | ||||||||
ID | Project | Category | View Status | Date Submitted | Last Update | ||||
0007000 | CMake | CPack | public | 2008-05-10 07:30 | 2010-09-09 23:48 | ||||
Reporter | Eric NOULARD | ||||||||
Assigned To | Eric NOULARD | ||||||||
Priority | normal | Severity | minor | Reproducibility | always | ||||
Status | closed | Resolution | fixed | ||||||
Platform | OS | OS Version | |||||||
Product Version | CMake-2-6 | ||||||||
Target Version | CMake 2.8.3 | Fixed in Version | CMake 2.8.3 | ||||||
Summary | 0007000: CPack RPM generator should always "CPACK_SET_DESTDIR" | ||||||||
Description | 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 | ||||||||
Tags | No tags attached. | ||||||||
Attached Files | ![]() ![]() ![]() ![]() ![]() ![]() ![]() | ||||||||
Relationships | |||||||||||||||||||||
|
Relationships |
Notes | |
(0011788) Eric NOULARD (developer) 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 (developer) 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 (developer) 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 (developer) 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 (developer) 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 (developer) 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 (reporter) 2008-07-29 09:30 |
Checking for IsSet("CPACK_SET_DESTDIR") and reporting error would be less surprise for regular user. |
(0013166) Eric NOULARD (developer) 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 (developer) 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 (developer) 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 (manager) 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 (manager) 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 (developer) 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 (developer) 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 (developer) 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 (developer) 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 (developer) 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 (developer) 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 (developer) 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 (developer) 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 (developer) 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 (developer) 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 (reporter) 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 (manager) 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 (developer) 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 (developer) 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 (developer) 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 (developer) 2010-08-24 16:36 |
The patch proposal has been pushed to next. |
(0022034) Eric NOULARD (developer) 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 |
Notes |
Issue History | |||
Date Modified | Username | Field | Change |
2008-05-10 07:30 | Eric NOULARD | New Issue | |
2008-05-10 07:30 | Eric NOULARD | File Added: myscript-1.0-src.tar.bz2 | |
2008-05-10 07:39 | Eric NOULARD | Note Added: 0011788 | |
2008-05-10 07:39 | Eric NOULARD | File Added: CPackRPM-SET_DESTDIR_On.patch | |
2008-05-10 17:44 | Alex Neundorf | Status | new => assigned |
2008-05-10 17:44 | Alex Neundorf | Assigned To | => Alex Neundorf |
2008-05-11 05:26 | Alex Neundorf | Note Added: 0011805 | |
2008-05-11 14:40 | Eric NOULARD | Note Added: 0011809 | |
2008-05-12 16:22 | Eric NOULARD | Note Added: 0011829 | |
2008-05-12 16:40 | Alex Neundorf | Note Added: 0011830 | |
2008-05-12 17:25 | Eric NOULARD | Note Added: 0011836 | |
2008-07-29 09:30 | Yuri | Note Added: 0012853 | |
2008-08-25 04:33 | Eric NOULARD | Note Added: 0013166 | |
2008-09-04 15:54 | Alex Neundorf | Note Added: 0013299 | |
2008-09-04 17:58 | Eric NOULARD | Note Added: 0013301 | |
2008-09-04 18:01 | Eric NOULARD | File Added: cpack_destdir_warn.patch | |
2008-09-05 10:57 | Brad King | Relationship added | related to 0007410 |
2008-09-05 10:59 | Brad King | Assigned To | Alex Neundorf => David Cole |
2008-09-05 11:12 | Brad King | Note Added: 0013303 | |
2008-09-05 11:15 | David Cole | Note Added: 0013304 | |
2008-09-06 14:35 | Eric NOULARD | Note Added: 0013333 | |
2008-09-06 19:25 | Eric NOULARD | Note Added: 0013335 | |
2008-09-06 19:25 | Eric NOULARD | File Added: cpack_destdir_warn-2.patch | |
2009-09-06 04:29 | Alex Neundorf | Note Added: 0017321 | |
2010-03-01 16:51 | Alex Neundorf | Note Added: 0019671 | |
2010-03-02 02:41 | Eric NOULARD | Note Added: 0019682 | |
2010-03-02 14:31 | Alex Neundorf | Note Added: 0019688 | |
2010-03-02 16:45 | Eric NOULARD | Note Added: 0019689 | |
2010-03-02 17:14 | Alex Neundorf | Note Added: 0019690 | |
2010-03-02 17:23 | Eric NOULARD | Note Added: 0019692 | |
2010-07-04 10:52 | Eric NOULARD | Relationship added | related to 0010701 |
2010-07-04 11:30 | Eric NOULARD | Relationship added | child of 0010935 |
2010-07-04 12:40 | Eric NOULARD | Note Added: 0021233 | |
2010-07-04 12:40 | Eric NOULARD | File Added: 0001-Authorize-warning-or-error-for-absolute-path-destina.patch | |
2010-07-06 02:38 | Eric NOULARD | Relationship added | related to 0010339 |
2010-08-07 09:21 | chriswolf | Note Added: 0021690 | |
2010-08-07 09:55 | chriswolf | Note Edited: 0021690 | |
2010-08-07 10:17 | chriswolf | Note Edited: 0021690 | |
2010-08-07 10:23 | chriswolf | Note Edited: 0021690 | |
2010-08-07 10:24 | chriswolf | Note Edited: 0021690 | |
2010-08-17 23:42 | David Cole | Note Added: 0021787 | |
2010-08-17 23:42 | David Cole | Assigned To | David Cole => Eric NOULARD |
2010-08-22 16:10 | Eric NOULARD | Note Added: 0021883 | |
2010-08-23 11:48 | Eric NOULARD | Note Added: 0021895 | |
2010-08-23 11:48 | Eric NOULARD | File Added: 0001-CPack-Backward-compatibly-enforce-DESTDIR-for-DEB-an.patch | |
2010-08-23 11:48 | Eric NOULARD | File Added: 0002-CPack-Enable-better-handling-of-absolute-installed-f.patch | |
2010-08-23 12:06 | Eric NOULARD | Note Added: 0021896 | |
2010-08-24 16:36 | Eric NOULARD | Note Added: 0021932 | |
2010-08-31 16:34 | Eric NOULARD | Note Added: 0022034 | |
2010-08-31 16:34 | Eric NOULARD | Status | assigned => closed |
2010-08-31 16:34 | Eric NOULARD | Resolution | open => fixed |
2010-08-31 16:34 | Eric NOULARD | Fixed in Version | => CMake-2-8 |
2010-08-31 18:03 | David Cole | Target Version | => CMake 2.8.3 |
2010-09-09 23:48 | David Cole | Fixed in Version | CMake-2-8 => CMake 2.8.3 |
Issue History |
Copyright © 2000 - 2018 MantisBT Team |