-
Notifications
You must be signed in to change notification settings - Fork 2
Fetching dependencies from source #3
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
Conversation
@@ -27,7 +27,7 @@ jobs: | |||
environment-name: build_env | |||
cache-environment: true | |||
- name: Install external dependencies | |||
run: micromamba install -n build_env -y -f ./environment-dev-external-deps.yml | |||
run: micromamba install -n build_env -y -f ./environment-dev.yml |
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 thought we wanted to separate tests with packages from conda and fetched from source? Do we eventually go with only using DFETCH_DEPENDENCIES_WITH_CMAKE
?
In that case, this job is useless and needs to be removed (installating packages is already done just before).
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
@@ -27,7 +27,7 @@ jobs: | |||
environment-name: build_env | |||
cache-environment: true | |||
- name: Install external dependencies | |||
run: micromamba install -n build_env -y -f ./environment-dev-external-deps.yml | |||
run: micromamba install -n build_env -y -f ./environment-dev.yml |
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.
Same here.
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
@@ -29,7 +29,7 @@ jobs: | |||
cache-environment: true | |||
init-shell: cmd.exe | |||
- name: Install external dependencies | |||
run: micromamba install -n build_env -y -f ./environment-dev-external-deps.yml | |||
run: micromamba install -n build_env -y -f ./environment-dev.yml |
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.
Same here.
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
runs-on: windows-latest | ||
strategy: | ||
matrix: | ||
build_type: [Debug] # TODO add Release |
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 removed when debugging, we should put it back.
build_type: [Debug] # TODO add Release | |
build_type: [Release, Debug] |
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
- name: Build sparrow-ipc | ||
run: | | ||
cmake -B build/ -G Ninja ^ | ||
-DCMAKE_BUILD_TYPE=${{ matrix.build_type }} ^ | ||
-DCMAKE_INSTALL_PREFIX=%CONDA_PREFIX% ^ | ||
-DCMAKE_PREFIX_PATH=%CONDA_PREFIX% ^ | ||
-DCMAKE_CXX_STANDARD=20 ^ |
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 a leftover from debugging as well, and is not supposed to be needed here.
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
-DCMAKE_BUILD_TYPE=${{ matrix.build_type }} ^ | ||
-DCMAKE_INSTALL_PREFIX=%CONDA_PREFIX% ^ | ||
-DCMAKE_PREFIX_PATH=%CONDA_PREFIX% ^ | ||
-DCMAKE_CXX_STANDARD=20 ^ |
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.
Same (should be removed).
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
if(NOT TARGET flatbuffers::flatbuffers) | ||
add_library(flatbuffers::flatbuffers ALIAS flatbuffers) | ||
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.
We should stick with one way of doing things, otherwise it's confusing. Either this way, or the way it was done for sparrow
:
if(TARGET sparrow)
target_link_libraries(sparrow-ipc PUBLIC sparrow)
elseif(TARGET sparrow::sparrow)
target_link_libraries(sparrow-ipc PUBLIC sparrow::sparrow)
else()
message(FATAL_ERROR "Could not find the sparrow target to link with.")
endif()
Feel free to chose what you prefer.
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
-DCMAKE_CXX_STANDARD=20 ^ | ||
-DSPARROW_IPC_BUILD_SHARED=${{ matrix.build_shared }} ^ | ||
-DFETCH_DEPENDENCIES_WITH_CMAKE=MISSING ^ | ||
-DCMAKE_VERBOSE_MAKEFILE=ON ^ |
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 we can remove this too (was for debugging).
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
# if(NOT FLATC_EXECUTABLE) | ||
# message(FATAL_ERROR "flatc not found. Please install Flatbuffers.") | ||
# 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.
If we decide to use directly flatc
without checking it's there beforehand, we should remove the commented parts then.
Same for find_program(FLATC_EXECUTABLE flatc)
.
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
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.
flatc is a target, so we don't have to call find_program
Replaced by #6 |
No description provided.