-
Notifications
You must be signed in to change notification settings - Fork 27
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
dont use global include dir #89
base: master
Are you sure you want to change the base?
Conversation
This is set so when installing header only libraries with |
I am just generally concerned with determinism, i.e. that the build of one package is not influenced by the presence of another, especcially for build caching. and also that when in cmake I use one package i dont get to see another "for free" so I can guarnatuee that only the specified dependencies are visible. header only should also be discoverable from the package dir, no? |
hmm, I cant find this header thing. -X is for specifying a cmake file, right? Ah, header.cmake :). Hmm, couldnt that produce a findable package? |
ok, I looked at header.cmake and tried to make it generate a config file, but I think its a bit too complex, also I dont really need it right now, so I will leave it alone. still i would like to avoid having all headers visible all the time, so I think I will just make this switchable |
This reverts commit 21cd83a.
ok, added --no-global-include to init fo this to be optional, ok? |
@@ -56,7 +56,8 @@ def w(obj, prefix, verbose, build_path, *args, **kwargs): | |||
@click.option('-D', '--define', multiple=True, help="Extra configuration variables to pass to CMake") | |||
@click.option('--shared', is_flag=True, help="Set toolchain to build shared libraries by default") | |||
@click.option('--static', is_flag=True, help="Set toolchain to build static libraries by default") | |||
def init_command(prefix, toolchain, cc, cxx, cflags, cxxflags, ldflags, std, define, shared, static): | |||
@click.option('--no-global-include', is_flag=True, help="Don't use global include dir (required for -X header)") |
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.
How about naming this --disable-global-include
?
Also, are you able to add a test for this? I think a simple test like this just to get coverage could be added in test/test_cget.py: def test_disable_global_include_toolchain(d):
d.cmds([
cget_cmd('init', '--disable-global-include'),
cget_cmd('install', '--verbose --test', get_exists_path('libsimple'))
]) |
DeepCode's analysis on #9a5780 found:
Top issues
👉 View analysis in DeepCode’s Dashboard | Configure the bot |
@@ -87,8 +90,10 @@ def init_command(prefix, toolchain, cc, cxx, cflags, cxxflags, ldflags, std, def | |||
@click.option('--debug', is_flag=True, help="Install debug version") | |||
@click.option('--release', is_flag=True, help="Install release version") | |||
@click.option('--insecure', is_flag=True, help="Don't use https urls") | |||
@click.option('--use-build-cache', is_flag=True, help="Cache builds") |
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.
This shouldn't have the build cache changes.
# TODO: Set also PKG_CONFIG_SYSROOT_DIR | ||
if(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE STREQUAL "ONLY") | ||
list(APPEND ${PREFIX}_BASE_ENV_COMMAND "PKG_CONFIG_LIBDIR=${${PREFIX}_PKG_CONFIG_PATH}") | ||
endif() |
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.
This shouldn't skip setting pkg config paths. Either way, I would prefer these changes in a seperate PR.
COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/autogen.sh | ||
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} | ||
) | ||
endif (AUTOTOOLS_RUN_AUTOGEN_SH) |
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.
This should be put in a seperate PR.
set_ = cmake_set | ||
if_ = cmake_if | ||
else_ = cmake_else | ||
append_ = cmake_append | ||
yield set_('CGET_PREFIX', self.prefix) | ||
yield set_('CMAKE_PREFIX_PATH', self.prefix) |
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.
Extending the CMAKE_PREFIX_PATH should not be removed.
its not neccessary and makes it harder to isolate components