-
Notifications
You must be signed in to change notification settings - Fork 592
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
Makefiles cleanup #3410
Makefiles cleanup #3410
Conversation
configs = $(filter-out $(SKIP),$(if $(filter all,$(CONFIGS)),$(supported-configs),$(filter $(supported-configs),$(CONFIGS)))) | ||
languages = $(filter-out $(SKIP),$(if $(filter all,$(or $(LANGUAGES),all)),$(supported-languages),$(filter $(supported-languages),$(LANGUAGES)))) | ||
build-platform := $(or $(build-platform),$(firstword $(supported-platforms))) | ||
|
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.
Added some comments to these macros to make clear what is the intend.
else | ||
cppflags += -std=c++17 | ||
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.
Don't need to care about these older clang versions.
@@ -160,8 +142,6 @@ cppflags = -fvisibility=hidden -Wall -Wextra -Wredundant-decls -Wshadow - | |||
$(if $(filter yes,$(OPTIMIZE)),-DNDEBUG,-g) | |||
ldflags = -pthread | |||
|
|||
gcc_version = $(shell $(CXX) -dumpversion 2>&1 | cut -f1 -d\.) |
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.
never used.
ifeq ($(shell pkg-config --exists libsystemd 2> /dev/null && echo yes),yes) | ||
Ice_system_libs += $(shell pkg-config --libs libsystemd) | ||
endif | ||
IceUtil_system_libs = -lrt -lbacktrace |
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.
lib backtrace and lib systemd should be available in all supported platforms
@@ -1,12 +1,5 @@ | |||
# Copyright (c) ZeroC, Inc. | |||
|
|||
# |
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 is just the same as project macro, don't to have both.
config/Make.rules.Linux
Outdated
Ice_system_libs += $(shell pkg-config --libs libsystemd) | ||
endif | ||
IceUtil_system_libs = -lrt -lbacktrace | ||
Ice_system_libs = -ldl -lssl -lcrypto $(shell pkg-config --libs libsystemd) |
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.
Missing IceUtil_system_libs
|
||
# system install directory | ||
system-install-dir := $(OECORE_TARGET_SYSROOT)/usr |
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.
Are these no longer needed?
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 was for Yocto, if we ever want to add back support for it we can bring it back. But so far we are not testing/supporting this with 3.8.
config/Make.rules.Linux
Outdated
Ice_system_libs += $(shell pkg-config --libs libsystemd) | ||
endif | ||
IceUtil_system_libs = -lrt -lbacktrace | ||
Ice_system_libs = -ldl -lssl -lcrypto $(IceUtil_system_libs) $(shell pkg-config --libs libsystemd) |
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.
what happens if there's no systemd, does $(shell pkg-config --libs libsystemd)
return empty?
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.
Do you think we should keep the check for libsystemd installed, or just emit an error if missing?
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.
Keep the check. For instance it's not going to almost ever be available in slim Docker 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.
Fixed as suggested
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.
Thanks!
This is an initial cleanup of our Makefiles