Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DESTDIR behavior at install time. #1235

Merged
merged 1 commit into from
Jan 24, 2017
Merged

Conversation

phcerdan
Copy link
Member

@phcerdan phcerdan commented Jan 20, 2017

We have to read DESTDIR at install time, not at configure time.
so we escape the $ symbol.
\$ENV{DESTDIR}

The same with TABLE_DIR inside configure_file.

Read more: https://cmake.org/pipermail/cmake-developers/2013-January/017810.html

Fix #1229

Checklist

  • [NA] Unit-test of your feature with Catch.
  • [NA] Doxygen documentation of the code completed (classes, methods, types, members...)
  • [NA] Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
  • All continuous integration tests pass (Travis & appveyor)

@copyme
Copy link
Member

copyme commented Jan 20, 2017

@phcerdan It is pity that this is not documented in CMake. I already mentioned about doc problems and DESTDIR via issue tracker of CMake. I hope it is going to be improved soon.

@phcerdan
Copy link
Member Author

@copyme yeah, they should document that install(CODE... and install(SCRIPT... don't manage DESTDIR by themselves. Thanks for that.
The tricky part here is that cmake generates a cmake_install.cmake at configure time, that will control the future installation. If we just put $ENV{DESTIR} it will be transformed to a string literal in cmake_install.cmake with the value at configure time of DESTIR (usually empty). But if we want cmake_install.cmake to read at install time $ENV{DESTIR}, it should be escaped.

I am not sure how well documented the escaping behavior is. But use of cpack requires two escape layers, (so for cpack to read \ it needs \\\), install(CODE...) requires one, etc...

@copyme
Copy link
Member

copyme commented Jan 22, 2017

@phcerdan I came to a conclusion that it would be better to start a new issue. See https://gitlab.kitware.com/cmake/cmake/issues/16583

@phcerdan
Copy link
Member Author

@copyme commented there as well.

@copyme
Copy link
Member

copyme commented Jan 23, 2017

@phcerdan I saw your comment. Thanks for the additional information!

@dcoeurjo
Copy link
Member

Hi @phcerdan & @copyme,
We're preparing a DGtal release and this PR is the last one that could be included. Have you found a solution for the @copyme's packing scripts on linux ?
Can I review this PR ?

@dcoeurjo dcoeurjo self-requested a review January 24, 2017 08:07
@dcoeurjo dcoeurjo added this to the 0.9.3 milestone Jan 24, 2017
@dcoeurjo dcoeurjo added the Build label Jan 24, 2017
@copyme
Copy link
Member

copyme commented Jan 24, 2017

@dcoeurjo I will make tests on build.opensuse.org, stay tuned ;-)

@phcerdan
Copy link
Member Author

phcerdan commented Jan 24, 2017

Hey @dcoeurjo definitely. This PR (of one character \) should fix current problem reading DESTDIR at install time. But @copyme tests might ensure that (thanks!)

@copyme
Copy link
Member

copyme commented Jan 24, 2017

@dcoeurjo @phcerdan I give you green light guys :-)

@dcoeurjo
Copy link
Member

Excellent ! ;)

@phcerdan, could you please add an tiny entry to the changelog (bug fix section) ?

The main drawback is that the commit size would drastically increased ;)

Copy link
Member

@dcoeurjo dcoeurjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changelog entry

We have to read DESTDIR at install time, not at configure time.
so we escpae the $ symbol.
`\$ENV{DESTDIR}`

The same with TABLE_DIR inside configure_file.
@phcerdan
Copy link
Member Author

done (rebased)

@dcoeurjo
Copy link
Member

👏

@dcoeurjo dcoeurjo merged commit dc8b334 into DGtal-team:master Jan 24, 2017
@phcerdan phcerdan deleted the lut-zlib branch January 28, 2017 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants