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

Management of GNU installation vars in CMake and Autotools #3599

Merged
merged 15 commits into from Oct 8, 2018

Conversation

@danimtb
Copy link
Member

@danimtb danimtb commented Sep 21, 2018

  • Refer to the issue that supports this Pull Request: closes #3594
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one. Also adding a description of the changes in the changelog.rst file. https://github.com/conan-io/docs

MERGE AFTER #3388

Changelog: Feature: The AutotoolsBuildEnvironment and CMake build helpers now adjust default for the GNU standard installation directories: bindir, sbin, libexec, includedir, oldincludedir, datarootdir

Changelog: Feature: Added use_default_install_dirs in AutotoolsBuildEnvironment.configure() to opt-out from the defaulted installation dirs.

@danimtb danimtb added this to the 1.8 milestone Sep 21, 2018
@ghost ghost assigned danimtb Sep 21, 2018
@ghost ghost added the stage: review label Sep 21, 2018
@danimtb danimtb removed their assignment Sep 24, 2018
@ghost ghost assigned danimtb Sep 27, 2018
@danimtb danimtb removed their assignment Sep 27, 2018
@danimtb danimtb requested a review from lasote Sep 27, 2018
@lasote lasote requested a review from memsharded Sep 27, 2018
lasote
lasote approved these changes Sep 27, 2018
Copy link
Contributor

@lasote lasote left a comment

It scares me a little but nothing against it.

@lasote lasote requested a review from jgsogo Sep 27, 2018
Copy link
Member

@jgsogo jgsogo left a comment

Remove extra lines and solve conflicts.

if not self._is_flag_in_args(varname, args):
args.append("--%s=${prefix}/%s" % (varname, DEFAULT_INCLUDE))
if not self._is_flag_in_args("datarootdir", args):
args.append("--datarootdir=${prefix}/%s" % DEFAULT_RES)

if not any(["--libdir=" in arg for arg in args]):
args.append("--libdir=${prefix}/lib")
Copy link
Member

@jgsogo jgsogo Sep 28, 2018

These lines are implicit in the lines 135-136

if not self._is_flag_in_args("libdir", args):
                args.append("--libdir=${prefix}/%s" % DEFAULT_LIB)

@ghost ghost assigned danimtb Oct 1, 2018
@danimtb
Copy link
Member Author

@danimtb danimtb commented Oct 1, 2018

CI failed because of ERROR: Issue with creating launcher for agent MacosBuilder. The agent has not been fully initialized yet

args.append("--libdir=${prefix}/lib")
for varname in ["bindir", "sbin", "libexec"]:
if not self._is_flag_in_args(varname, args):
args.append("--%s=${prefix}/%s" % (varname, DEFAULT_BIN))
Copy link
Contributor

@SSE4 SSE4 Oct 3, 2018

does it mean --bindir will now be always added to the configure command line?
I worry about libraries which provide autotools-like configure scripts, which are not really auto-tools based (e.g. ffmpeg, libvpx, etc.)
at least there should be way to avoid adding these arguments from recipe (e.g. like declaring build/host/target to None)

Copy link
Member Author

@danimtb danimtb Oct 7, 2018

I think defining the output folders is something mandatory for the build helpers and this behavior should no rely on the platform where the recipe is being build. Will have to aware users that this might be breaking for the cases you pointed out. i will have a look to see if there is any way to opt-out

Copy link
Contributor

@SSE4 SSE4 Oct 8, 2018

yes, opt-out would be nice to have, at least to let hand-crafted configure scripts to continue working in conan environment.

Copy link
Member

@memsharded memsharded Oct 8, 2018

Yes, please opt-out in the build helper.

"CMAKE_INSTALL_LIBDIR": "lib",
"CMAKE_INSTALL_INCLUDEDIR": "include",
"CMAKE_INSTALL_OLDINCLUDEDIR": "include",
"CMAKE_INSTALL_DATAROOTDIR": "res"}
Copy link
Contributor

@SSE4 SSE4 Oct 3, 2018

any reason to use "res" instead of "share" for datarootdir? I would expect it will require lots of programs to change location then

Copy link
Member Author

@danimtb danimtb Oct 7, 2018

"res" is the default folder name for resources and data files in self.cpp_info https://docs.conan.io/en/latest/reference/conanfile/methods.html#cpp-info

Copy link
Contributor

@SSE4 SSE4 Oct 8, 2018

in this case, may be it's better to add "shared" as a secondary default in addition to the existing "res"? "res" sounds to be too foreign for autotools-based installations.

Copy link

@wittmeie wittmeie Oct 9, 2018

Often CMake package-config files are installed to /share
install( FILES ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}-config.cmake ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}-config-version.cmake DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/cmake/${PROJECT_NAME} )

With this change, they are installed to /res which is not a default search path of the CMake find_package command: https://cmake.org/cmake/help/v3.12/command/find_package.html?highlight=find_package

Hence, packages are not found anymore if included via conan_basic_setup()!!!

Copy link
Member Author

@danimtb danimtb Oct 10, 2018

You were right, I am proposing a fix for this in #3705

Great - many thanks!

lasote
lasote approved these changes Oct 8, 2018
@@ -95,7 +96,7 @@ def _get_host_build_target_flags(self):
return build, host, None

def configure(self, configure_dir=None, args=None, build=None, host=None, target=None,
pkg_config_paths=None, vars=None):
pkg_config_paths=None, vars=None, default_install_dirs=True):
Copy link
Member

@jgsogo jgsogo Oct 8, 2018

default_install_dirs > use_default_install_dirs, I think it is more explicit if it is a True/False parameter.

Copy link
Member Author

@danimtb danimtb Oct 8, 2018

Thought about it but I think it is too long, however the boolean type could be more easily inferred

@danimtb
Copy link
Member Author

@danimtb danimtb commented Oct 8, 2018

Done!

@danimtb danimtb closed this Oct 8, 2018
@ghost ghost removed the stage: review label Oct 8, 2018
@danimtb danimtb reopened this Oct 8, 2018
@ghost ghost added the stage: review label Oct 8, 2018
jgsogo
jgsogo approved these changes Oct 8, 2018
@memsharded memsharded merged commit 0a025da into conan-io:develop Oct 8, 2018
2 checks passed
@ghost ghost removed the stage: review label Oct 8, 2018
grisumbras pushed a commit to grisumbras/conan that referenced this issue Dec 27, 2018
Management of GNU installation vars in CMake and Autotools
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants