Add uninstall target #35
Conversation
|
@TheFixer, thanks for this pull request, but I can't merge it yet. You will have to amend the commit to meet the Eclipse IP validation rules (you have to add a Signed-off-by: ... line with your own name and email) before it can be accepted. Secondly, could you please add a test that verifies that the uninstall actually deletes the installed files and gives a reasonable indication that it does not try to delete any others? Especially the latter is important to me. |
|
I'd like to investigate the options a little further. I'm not sure if the install target works on Windows for example (I think it does), so we should then add it for Windows as well. Also, we should remove the make install command from the README.md and replace make by cmake --build . because that works for every generator. |
|
Please change the location of cmake_uninstall.cmake.in to src/cmake/cmake_uninstall.cmake.in. That's a much better location. Then update the build instructions to include the following. Ensure that you also test the uninstall target on at least Linux and Windows and add that to the instructions as well. Windows: Linux: And last, but not least, please add that people need to think about whether they actually want to install this to their system. We should mention that it's not required to run the examples. Also, i found that the dds_idlc script is not copied. Instead dds_idlc.in is copied, that should be changed. Can you fix that while you're at it? |
TheFixer
force-pushed
the
create-uninstall-target
branch
from
72386b7
to
775cf45
Compare
Signed-off-by: TheFixer <thefixer@xs4all.nl>
Signed-off-by: TheFixer <thefixer@xs4all.nl>
…users and contributors Signed-off-by: TheFixer <thefixer@xs4all.nl>
TheFixer
force-pushed
the
create-uninstall-target
branch
from
775cf45
to
fe67765
Compare
|
Thanks for the comments. I followed up on them as follows:
Below some evidence is provided that the uninstall target works.
|
| On Windows: | ||
|
|
||
| $ cd build | ||
| $ cmake -DCMAKE_BUILD_TYPE=<build-type> -G "<generator-name>" -DCMAKE_INSTALL_PREFIX=<install-location> ../src |
There was a problem hiding this comment.
Perhaps it would be better to give a sensible suggestion for the generator on Windows? Like:
| $ cmake -DCMAKE_BUILD_TYPE=<build-type> -G "<generator-name>" -DCMAKE_INSTALL_PREFIX=<install-location> ../src | |
| $ cmake -DCMAKE_BUILD_TYPE=<build-type> -G "Visual Studio 15 2017 Win64" -DCMAKE_INSTALL_PREFIX=<install-location> ../src |
or some other following the -G option if another version of Visual Studio or a 32-bit build is desired.
There was a problem hiding this comment.
@eboasson Since you are asking my opinion ... I can image that you would like to have a concrete example of a cmake install command and thus want a concrete value for the generator name. But then I would also expect a concrete value for the build-type. It looks a bit arbitrary to me provide a concrete value for the generator name, but not for the build-type.
There was a problem hiding this comment.
Fair point — one approach for that is to remove the CMAKE_BUILD_TYPE setting from the example, and mention the option and the fact that it defaults to RelWithDebInfo.
| endif(NOT EXISTS "@CMAKE_BINARY_DIR@/install_manifest.txt") | ||
|
|
||
| file(READ "@CMAKE_BINARY_DIR@/install_manifest.txt" files) | ||
| string(REGEX REPLACE "\n" ";" files "${files}") |
There was a problem hiding this comment.
I see potential for interesting results here if the file names or the installation directory contains newlines or semicolons ... but ... I think it is fair to promise that neither the filenames nor the default installation directory will contain tricky character sequences.
So on consideration, I'm ok with committing this code. (I would have expected better from cmake though.)
There was a problem hiding this comment.
@eboasson I think you have a valid point here. There is nothing to prevent a user to use an install path contain weird directory names (e.g., directory names including \n). First question would be if we can install successfully. If so, we should also uninstall correctly.
There was a problem hiding this comment.
@eboasson I did a quick check to see if you can install in a "weird" directory. What I did is the following
$ mkdir "build\ntest"
$ cd "build\ntest"
$ cmake -DCMAKE_BUILD_TYPE=Release ../src
Even though "build\ntest" is a valid directory, installation fails miserably.
I also did a test where the CMAKE_INSTALL_PREFIX contains a weird directory containing a \n. Installing fails miserably too with the following error:
-- CycloneDDS documentation: disabled (-DUSE_DOCS=1 to enable)
-- Configuring incomplete, errors occurred!
See also "/home/lex/Repositories/TheFixer/cyclonedds/build.1/CMakeFiles/CMakeOutput.log".
See also "/home/lex/Repositories/TheFixer/cyclonedds/build.1/CMakeFiles/CMakeError.log".
Makefile:458: recipe for target 'cmake_check_build_system' failed
make: *** [cmake_check_build_system] Error 1
My conclusion is that it is currently pretty difficult, if not impossible, to install CycloneDDS in a directory containing a \n
There was a problem hiding this comment.
My point was actually that one has to configure it in a really weird way, one unlikely to happen unintentially, and that it therefore is not a big deal. But it is really nice that you have actually spent time checking how it (mis)behaves :)
Co-Authored-By: TheFixer <lex.heerink@adlinktech.com>
|
|
||
| ### For contributors | ||
|
|
||
| To contribute to the development of Eclipse Cyclone DDS you may need to provide test cases using [cunit](http://cunit.sourceforge.net/). Once cunit is installed do |
There was a problem hiding this comment.
Please instruct users/committers to use Conan.io, because they need Criterion as well as CUnit. Using Conan.io is much, MUCH simpler. Please also state that they don't necessarily need Conan and that manual installation or installation through package management is also an option and works. It's just that Criterion isn't available on any distribution by default.
There was a problem hiding this comment.
@k0ekk0ek, I agree with this being valuable (indeed, I use it, too, it saves a lot of hassle) and I would welcome it included in the pull request, but I'm not inclined to require it to be included. The reason is that I would rather accept a change that is a real improvement and then continue adding to that, then delay the suggested improvements in search of something better still.
|
|
||
| $ cd build | ||
| $ cmake -DCMAKE_BUILD_TYPE=<build-type> -DBUILD_TESTING=on -DCMAKE_INSTALL_PREFIX=<install-location> ../src | ||
| $ cmake --build . --target install |
There was a problem hiding this comment.
Please remove the --target install from the default instructions. Or, alternatively, state that it's not recommended or something.
There was a problem hiding this comment.
@k0ekk0ek I noticed the install thing, too, but I don't quite see what the harm is considering the presence of -DCMAKE_INSTALL_PREFIX=<install-location> and I noticed myself that there is a difference between installing & not installing if one intends to use Cyclone DDS for some other project.
Co-Authored-By: TheFixer <lex.heerink@adlinktech.com>
|
Closing this one because the bulk of points it raised are included in PR#114. The uninstall target isn't: not because I was against it, but because my try to cherry-pick caused the PR to be rejected by the Eclipse IP validation. |
This pull requests adds an uninstall target that allows a user to uninstall cyclonedds if desired.
When a user installs Cyclone using "make install" then various executables, libraries and other files that have been generated as result of the build process are being deployed on the user's system. The list of files that are generated can be found in the install_manifest.txt. Unfortunately, cmake does not generate an uninstall target by itself, so in order to clean up cyclonedds an uninstall target has to be provided explicitly.
The uninstall target in this commit provides the user with the option to run "make uninstall" . The effect of this is that all files listed in the install_manifest.txt will be removed, so that the user does not have any leftovers of cyclonedds when the user decides to uninstall it.
To activate the uninstaller simply use " make uninstall" in the build directory. To perform the uninstall the user must need the proper privileges to do so (so on Linux you might have to use 'sudo make uninstall'