Skip to content

cmake: resolve build issues#18

Merged
edsiper merged 4 commits into
masterfrom
17_add_extra_buildsc
Sep 15, 2022
Merged

cmake: resolve build issues#18
edsiper merged 4 commits into
masterfrom
17_add_extra_buildsc

Conversation

@patrick-stephens
Copy link
Copy Markdown

@patrick-stephens patrick-stephens commented Sep 15, 2022

Resolves #17
Missing variables were triggering failures so have added those and also provided an extra test case.

Added linting plus sanitiser calls for unit tests.

Patrick Stephens added 4 commits September 15, 2022 10:32
Signed-off-by: Patrick Stephens <pat@calyptia.com>
Signed-off-by: Patrick Stephens <pat@calyptia.com>
Signed-off-by: Patrick Stephens <pat@calyptia.com>
Signed-off-by: Patrick Stephens <pat@calyptia.com>
Comment thread CMakeLists.txt
set(CFL_INSTALL_BINDIR "bin")
set(CFL_INSTALL_LIBDIR "lib")
else()
set(CFL_INSTALL_BINDIR ${CMAKE_INSTALL_FULL_BINDIR})
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just followed what Fluent Bit did here but you may want another specific directory.

- uses: actions/checkout@v3
- uses: ludeeus/action-shellcheck@master
with:
ignore_paths: lib
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scripts in xxhash have a few "issues" so ignoring any dependencies here.

env:
CC: ${{ matrix.compiler }}

build-analysis-tests:
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken from cmetrics/ctraces

postgresql-libs postgresql-devel postgresql-server postgresql && \
wget http://dl.fedoraproject.org/pub/epel/epel-release-latest-7.noarch.rpm && \
rpm -ivh epel-release-latest-7.noarch.rpm && \
yum -y update
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified and reduced dependencies required to speed up builds

- name: Run compilation
run: |
cmake -DCFL_DEV=on .
make
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this run in build/?
Could you use cmake --build . instead of make?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah can do, I just left it as it was before in the other builds.

@@ -75,6 +91,7 @@ jobs:
CTEST_OUTPUT_ON_FAILURE=1 make test
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we use ctest for this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fine, since cmake commands generate a Makefile test command that uses ctest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that doesn't work in windows right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a Linux build

@edsiper edsiper merged commit ebf50a8 into master Sep 15, 2022
@edsiper edsiper deleted the 17_add_extra_buildsc branch September 15, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build failures on older targets

3 participants