Draft PR covering Canonical packaging dev changes #466
Conversation
Signed-off-by: Liu, Xiangquan <[email protected]>
* Implement the proposed solution of library versioning note: main branch missing a delete of unused QVE_VER?
note: to be merged * Update build system to link system libcrypto Signed-off-by: Xiangquan Liu <[email protected]>
| USE_PREBUILT_OPENSSL ?= 0 | ||
| ifeq ($(USE_PREBUILT_OPENSSL), 0) | ||
| CRYPTO_LIB = $(shell pkg-config --libs libcrypto 2>/dev/null) | ||
| CRYPTO_INC = $(shell pkg-config --cflags libcrypto 2>/dev/null) |
There was a problem hiding this comment.
Adding warning about potential pkg-config issues (and/or not installed) to be considered plus fallback mechanism:
| CRYPTO_INC = $(shell pkg-config --cflags libcrypto 2>/dev/null) | |
| CRYPTO_INC = $(shell pkg-config --cflags libcrypto 2>/dev/null) | |
| # Fallback if pkg-config fails or not installed | |
| ifeq ($(CRYPTO_LIB),) | |
| CRYPTO_LIB := -lcrypto | |
| endif | |
| ifeq ($(CRYPTO_INC),) | |
| CRYPTO_INC := | |
| endif |
There was a problem hiding this comment.
I think in case CRYPTO_LIB and CRYPTO_INC cannot be retrieved with pkg-config, we can stop and output an error message
| -I$(DCAP_TOPDIR)/QuoteGeneration/common/inc/internal/ \ | ||
| -I$(DCAP_TOPDIR)/QuoteGeneration/common/inc/internal/linux/ \ | ||
| -I$(PREBUILD_OPENSSL_PATH)/inc \ | ||
| -I../common |
There was a problem hiding this comment.
| -I../common | |
| -I../common \ |
There was a problem hiding this comment.
Thanks @bgotowal for this fix, Lgtm
| ifneq ($(wildcard $(SGX_SDK)),) | ||
| include $(SGX_SDK)/buildenv.mk | ||
| else ifneq ($(wildcard /usr/share/sgxsdk/buildenv.mk),) | ||
| include /usr/share/sgxsdk/buildenv.mk |
There was a problem hiding this comment.
It would be beneficial to have access to proposed buildenv.mk used in the local system. That would clarify which areas are intended to be configurable and enable Intel to provide more comprehensive test coverage. This might help prevent accidental regressions or issues affecting the build.
| INCLUDE += -I. -I../inc | ||
| INCLUDE += -I$(SGX_SDK)/include \ | ||
| -I$(COMMON_DIR)/inc/internal \ | ||
| INCLUDE += -I$(SGX_TRUSTED_INCLUDE_PATH) \ |
There was a problem hiding this comment.
Can we have access to specific location (path) and content of the specific SDK include path? Is it simply a regular SGX SDK's include directory (built from Open Source or installed from Intel provided bin) but located outside of default installation location? E.g., SDK sub-dirs are located in Ubuntu-specific areas depending on the nature of the files?
|
|
||
| CFLAGS += -fPIC -Werror -g | ||
| Link_Flags := $(SGX_COMMON_CFLAGS) -L$(ROOT_DIR)/build/linux -L$(SGX_SDK)/lib64 -lsgx_urts -lpthread -ldl | ||
| Link_Flags := $(SGX_COMMON_CFLAGS) -L$(ROOT_DIR)/build/linux -L$(SGX_LIBRARY_PATH) -lsgx_urts -lpthread -ldl |
There was a problem hiding this comment.
Can we have access to specific location (path) and content of the specific SDK library path? Is it simply a regular SGX SDK's library directory (built from Open Source or installed from Intel provided bin) but located outside of default installation location? E.g., SDK sub-dirs are located in Ubuntu-specific areas depending on the nature of the files?
There was a problem hiding this comment.
Could you elaborate a little bit more on your question ?
The idea of this diff, I believe, is to use the right variable SGX_LIBRARY_PATH that points to the SGX libs
| p_eid, | ||
| NULL); | ||
| if (SGX_SUCCESS != sgx_status) { | ||
| #if defined(__GNUC__) |
There was a problem hiding this comment.
While #if defined(__GNUC__) will be true for many supported OSes, the solution provided std::string enclave_path_for_ubuntu("/usr/lib/x86_64-linux-gnu/"); is for debian systems only. We might want to consider extending the coverage.
No description provided.