-
Notifications
You must be signed in to change notification settings - Fork 993
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It scares me a little but nothing against it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
CI failed because of |
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, opt-out would be nice to have, at least to let hand-crafted configure scripts to continue working in conan environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please opt-out in the build helper.
"CMAKE_INSTALL_LIBDIR": "lib", | ||
"CMAKE_INSTALL_INCLUDEDIR": "include", | ||
"CMAKE_INSTALL_OLDINCLUDEDIR": "include", | ||
"CMAKE_INSTALL_DATAROOTDIR": "res"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to use "res" instead of "share" for datarootdir? I would expect it will require lots of programs to change location then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were right, I am proposing a fix for this in #3705
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great - many thanks!
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default_install_dirs
> use_default_install_dirs
, I think it is more explicit if it is a True/False parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about it but I think it is too long, however the boolean type could be more easily inferred
Done! |
Management of GNU installation vars in CMake and Autotools
develop
branch, documenting this one. Also adding a description of the changes in thechangelog.rst
file. https://github.com/conan-io/docsMERGE AFTER #3388
Changelog: Feature: The
AutotoolsBuildEnvironment
andCMake
build helpers now adjust default for the GNU standard installation directories:bindir
,sbin
,libexec
,includedir
,oldincludedir
,datarootdir
Changelog: Feature: Added
use_default_install_dirs
inAutotoolsBuildEnvironment.configure()
to opt-out from the defaulted installation dirs.