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

fix to MCOL-2110 Cant build engine out-of-source #681

Merged
merged 2 commits into from Jan 28, 2019

Conversation

djmott
Copy link
Contributor

@djmott djmott commented Jan 25, 2019

Prefixing SERVER_BUILD_INCLUDE_DIR and SERVER_SOURCE_ROOT_DIR with CMAKE_BINARY_DIR works in the case of the engine and server build folders being siblings to each other and relative paths supplied to cmake. If the build folders are not siblings and absolute paths are supplied to cmake it fails. Using GET_FILENAME_COMPONENT will resolve the correct path in both cases.

Prefixing SERVER_BUILD_INCLUDE_DIR and SERVER_SOURCE_ROOT_DIR with CMAKE_BINARY_DIR works in the case of the engine and server build folders being siblings to each other and relative paths supplied to cmake. If the build folders are not siblings and absolute paths are supplied to cmake it fails. Using GET_FILENAME_COMPONENT will resolve the correct path in both cases.
@djmott
Copy link
Contributor Author

djmott commented Jan 25, 2019

I am submitting this patch under the BSD-new license.

@drrtuy
Copy link
Collaborator

drrtuy commented Jan 25, 2019

Greetings David.
Thank you for the contribution we appreciate your support.
The change makes our buildbot b/c it didn't account for the change. We will look into this problem.

@djmott
Copy link
Contributor Author

djmott commented Jan 25, 2019

Sorry I didn't mean to make work for you. Building out-of-source according to the README.md worked for me also building from within subdirectories of the respective projects while passing absolute paths of the servers artifacts to cmake for the engine. I use some tools that need cmake configured this way. I'll take a look and try to replicate. If its a problem just don't merge, I'm happy just keeping it local.

The interesting CI steps are shell and shell_6:
shell:
PWD=/data/buildbot/bb-worker/centos6_PR/build/mariadb-columnstore-server
cmake ../../mariadb-columnstore-server

shell_6:
PWD=/data/buildbot/bb-worker/centos6_PR/build/mariadb-columnstore-engine
cmake ../../mariadb-columnstore-engine -DSERVER_BUILD_INCLUDE_DIR=../mariadb-columnstore-server/include -DSERVER_SOURCE_ROOT_DIR=../../mariadb-columnstore-server'

...

SERVER_BUILD_INCLUDE_DIR = /data/buildbot/bb-worker/centos6_PR/mariadb-columnstore-server/include
SERVER_SOURCE_ROOT_DIR = /data/buildbot/bb-worker/mariadb-columnstore-server

@djmott
Copy link
Contributor Author

djmott commented Jan 25, 2019

I see, it's expecting the the paths to be relative to CMAKE_CURRENT_BINARY_DIR instead of CMAKE_CURRENT_LIST_DIR. It works according to README.md because the relative paths to the server artifacts in SERVER_BUILD_INCLUDE_DIR are the same. I'll fix.

@djmott djmott closed this Jan 25, 2019
@djmott djmott reopened this Jan 25, 2019
@drrtuy
Copy link
Collaborator

drrtuy commented Jan 28, 2019

It looks great now.

@drrtuy drrtuy self-requested a review January 28, 2019 16:23
@drrtuy drrtuy merged commit 13398fa into mariadb-corporation:develop-1.2 Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants