Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e75c8a4
cmake: use add_custom_command(OUTPUTS xx.h) + add_executable(.. xx.h)…
madebr Dec 16, 2020
e231644
cmake: install cmake targets
madebr Dec 16, 2020
17b346a
cmake: use alias'ed flatcc::(cli|libflatcc|runtime)
madebr Dec 16, 2020
7d836bb
cmake: generate + install flatcc-config.cmake
madebr Dec 16, 2020
30bfbaf
cmake: use add_Test(NAME .. COMMAND ..) to support commands with exec…
madebr Dec 16, 2020
c7d5302
cmake: add custom target for parallel MSVC builds
madebr Dec 16, 2020
cb6563e
Enable test_generated test
madebr Dec 16, 2020
6fdd281
Create cmake subfolder
madebr Dec 16, 2020
519ed9a
#168: add cmake helper function to generate flatbuffer code
Dec 16, 2020
5f03ecd
Use flatcc_generate_source to generate headers + track dependencies
madebr Dec 17, 2020
90202ba
cmake: don't use file(REAL_PATH)
madebr Dec 17, 2020
a40d6c3
ci: output message when test fails
madebr Dec 17, 2020
199bcac
cmake: apply regex on sources to track dependencies
madebr Dec 17, 2020
40174a7
cmake: install flatcc-config-version.cmake to allow version discrimin…
madebr Dec 17, 2020
1f0ab8f
cgen_test: buill test code as library (to make use of cmake build sys…
madebr Dec 17, 2020
d2322c8
ci: add github actions script
madebr Dec 17, 2020
41b1a5f
fix test_generated
madebr Dec 17, 2020
38acb79
remove lib/libtest* libraries in scripts/cleanall.sh
madebr Mar 27, 2021
10f6a9e
ci: add cross building github action
madebr Mar 27, 2021
c1f1fd9
Add .editorconfig
madebr Mar 27, 2021
9808df5
Restored changed after botched force push
madebr Mar 27, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
root = true

[{*.c, *.h, *.cpp, *.fbs, *.sh, *.cmake, *.cmake.in, *.txt}]
indent_style = space
indent_size = 4
insert_final_newline = true
trim_trailing_whitespace = true

[{*.yml, *.json}]
indent_style = space
indent_size = 2
insert_final_newline = true
trim_trailing_whitespace = true
52 changes: 52 additions & 0 deletions .github/workflows/ci-more.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
on:
schedule:
# Run this script every saturday, so you have the whole weekend to fix problems :evil-grin:
- cron: '0 0 * * 6'
jobs:
merge_into_ci_more:
# Only enable this job on dividelabs' repo to avoid running this job on forks
if: ${{ github.repository_owner == 'dividelabs' }}
runs-on: 'ubuntu-latest'
steps:
- name: 'Checkout repo'
uses: actions/checkout@v2
with:
fetch-depth: 0
- name: 'Configure git'
run: |
git config --global user.name "ci-more cron job"
git config --global user.email "ci-more-cron-job@example.com"
- name: 'Checkout ci-more'
run: |
git checkout ci-more
- name: 'Backup .travis.yml and appveyor.yml'
run: |
cp .travis.yml .travis.yml.backup
cp appveyor.yml appveyor.yml.backup
- name: 'Checkout .travis.yml and appveyor.yml from master'
run: |
git checkout origin/master -- .travis.yml appveyor.yml
git add .travis.yml appveyor.yml
git commit -m "Temporarily add .travis.yml and appveyor.yml from master"
- name: 'Merge master into ci-more'
run: |
git merge origin/master -m "Merge master into ci-more"
- name: 'Restore .travis.yml and appveyor.yml'
run: |
mv .travis.yml.backup .travis.yml
mv appveyor.yml.backup appveyor.yml
git add -- .travis.yml appveyor.yml
git commit -m "Restore .travis.yml and appveyor.yml"
- name: 'Check whether the working directory is clean'
run: |
git diff --exit-code
- name: 'Check whether only .travis.yml and appveyor.yml differ with master'
run: |
if test $(git diff origin/master --name-only | wc -l) != 2; then
echo "ci-more should diverge from origin/master with exactly 2 files (.travis.yml and appveyor.yml)";
git diff origin/master --exit-code; # origin/master
exit 1;
fi
Comment on lines +22 to +49
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.

When you end up removing .travis.yml and appveyor.yml from master, then ci-more.yml can be reduced.

Suggested change
- name: 'Backup .travis.yml and appveyor.yml'
run: |
cp .travis.yml .travis.yml.backup
cp appveyor.yml appveyor.yml.backup
- name: 'Checkout .travis.yml and appveyor.yml from master'
run: |
git checkout origin/master -- .travis.yml appveyor.yml
git add .travis.yml appveyor.yml
git commit -m "Temporarily add .travis.yml and appveyor.yml from master"
- name: 'Merge master into ci-more'
run: |
git merge origin/master -m "Merge master into ci-more"
- name: 'Restore .travis.yml and appveyor.yml'
run: |
mv .travis.yml.backup .travis.yml
mv appveyor.yml.backup appveyor.yml
git add -- .travis.yml appveyor.yml
git commit -m "Restore .travis.yml and appveyor.yml"
- name: 'Check whether the working directory is clean'
run: |
git diff --exit-code
- name: 'Check whether only .travis.yml and appveyor.yml differ with master'
run: |
if test $(git diff origin/master --name-only | wc -l) != 2; then
echo "ci-more should diverge from origin/master with exactly 2 files (.travis.yml and appveyor.yml)";
git diff origin/master --exit-code; # origin/master
exit 1;
fi
- name: 'Merge master into ci-more'
run: |
git merge origin/master -m "Merge master into ci-more"
- name: 'Check whether the working directory is clean'
run: |
git diff --exit-code
- name: 'Check whether only .travis.yml and appveyor.yml differ with master'
run: |
if test "$(git diff origin/master --name-only | xargs)" != ".travis.yml appveyor.yml"; then
git diff origin/master --exit-code;
fi

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.

Please check all options of the function, because once added (and released),
it's very hard (not to say impossible) to remove them.

It's hard for me to tell if this is implemented correctly, so I am going (mostly) for the documentation. I really don't know how CMake operates in this context.

Note, I might deprecate --schema and rename to --binary-schema or --reflection in a future flatcc version, but that doesn't change much here.

Looking at the comments in FlatccGenerateSources.cmake:

I wrote earlier that I'm not sure that setting READER as the default argument. But it also seems that ALL, RECURSIVE is being set, which I think is more useful:

set(output_options BINARY_SCHEMA COMMON COMMON_READER COMMON_BUILDER BUILDER READER VERIFIER JSON_PARSER JSON_PRINTER JSON)
set(NO_VAL_ARGS ALL RECURSIVE ${output_options})
set(SINGLE_VAL_ARGS NAME OUTPUT_DIR OUTFILE PREFIX)
set(MULTI_VAL_ARGS SCHEMA_FILES COMPILE_FLAGS PATHS)

But later the following section says differently:

if(default_option_reader)
set(FLATCC_READER ON)
endif()

There is an old error message here, should be "No flatbuffer schema files provided":

if (NOT FLATCC_SCHEMA_FILES)
message(FATAL_ERROR "No flatbuffer definition files provided")
endif()

I don't understand this line:

set(MULTI_VAL_ARGS SCHEMA_FILES COMPILE_FLAGS PATHS)

Later you append -I prefix to FLATCC_PATHS - I'm note sure what a keyword args on other CMake context.

Regarding the COMPILE_FLAGS option, this is (probably) correct, but I wonder if EXTRA_ARGS, EXTRA_FLAGS, or something else would be more meaningful.

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.

Note, I might deprecate --schema and rename to --binary-schema or --reflection in a future flatcc version, but that doesn't change much here.

The function now accepts BINARY_SCHEMA only.
This function/file is meant to be distributed along flatcc, so it can be kept in sync.
When some options changes in the future, there are ways to ease the transition.

Looking at the comments in FlatccGenerateSources.cmake:

I wrote earlier that I'm not sure that setting READER as the default argument. But it also seems that ALL, RECURSIVE is being set, which I think is more useful:

Is ALL the new agreed-upon default for cmake then?
You can also choose for a non-standard option e.g. COMMON + BUILDER + VERIFIER + READER without RECURSIVE.
But I think this will be harder to document, for no gain.

set(output_options BINARY_SCHEMA COMMON COMMON_READER COMMON_BUILDER BUILDER READER VERIFIER JSON_PARSER JSON_PRINTER JSON)
set(NO_VAL_ARGS ALL RECURSIVE ${output_options})
set(SINGLE_VAL_ARGS NAME OUTPUT_DIR OUTFILE PREFIX)
set(MULTI_VAL_ARGS SCHEMA_FILES COMPILE_FLAGS PATHS)

But later the following section says differently:

if(default_option_reader)
set(FLATCC_READER ON)
endif()

What this does, is iterate over all output_options (ALL BINARY_SCHEMA COMMON COMMON_READER COMMON_BUILDER BUILDER READER) and if none of these are set, then assume the caller has passed READER (=default output option when no output has been selected).

There is an old error message here, should be "No flatbuffer schema files provided":

if (NOT FLATCC_SCHEMA_FILES)
message(FATAL_ERROR "No flatbuffer definition files provided")
endif()

Fixed (also removed all reference of definition file to schema file).

I don't understand this line:

set(MULTI_VAL_ARGS SCHEMA_FILES COMPILE_FLAGS PATHS)

This variable is later used in cmake_parse_arguments.
It is a list of arguments that except multiple arguments.
SCHEMA_FILES can be one file or more.

Later you append -I prefix to FLATCC_PATHS - I'm note sure what a keyword args on other CMake context.

Regarding the COMPILE_FLAGS option, this is (probably) correct, but I wonder if EXTRA_ARGS, EXTRA_FLAGS, or something else would be more meaningful.

I copied the terminology from FindBISON. But I guess EXTRA_FLAGS is ok too.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I created another PR to @madebr 's branch.

Things still to do/check:

  • When running cmake with BUILD_SHARED_LIBS=On the installed flatcc exe can't find the flatcc compiler shared lib (on Linux) unless you set LD_LIBRARY_PATH. I think we should add RPATH.
  • Cross-compiling and Conan compatibility to be checked

If you can postpone the merge to Tuesday or so, I can do that.

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.

@wdobbe

I'm in no hurry to merge.

@madebr

If the READER is default, what does the following mean?

set(NO_VAL_ARGS ALL RECURSIVE ${output_options})

Is ALL the new agreed-upon default for cmake then?

No I am raising the discussion. I would expect at least READER + COMMON_READER as default because without a COMMON_READER the result will not be usable unless the common file is created elsewhere.

But in general ALL + RECURSIVE will build anything a user would expect when not caring too much about arguments, which is why I think that will be a good choice. But I am open to suggestions.

It looks to me as if you expand all the -I paths without the -I prefix and later try to parse it, but I am probable just not understanding the mechanism:

set(MULTI_VAL_ARGS SCHEMA_FILES COMPILE_FLAGS PATHS)
This variable is later used in cmake_parse_arguments.

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 created another PR to @madebr 's branch.

Thanks! And merged!

* When running cmake with BUILD_SHARED_LIBS=On the installed flatcc exe can't find the flatcc compiler shared lib (on Linux) unless you set LD_LIBRARY_PATH. I think we should add RPATH.

This is added, and tested.
See https://github.com/madebr/flatcc/actions/runs/432848623

* Cross-compiling and Conan compatibility  to be checked

I'll leave that up to you :D

- name: 'Push ci-more to server'
run: |
git push origin ci-more
183 changes: 183 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
on:
push:
branches-ignore:
- 'ci-more'
pull_request:
jobs:
test_unix:
name: 'Test Unix with CMake'
strategy:
matrix:
include:
- os: 'ubuntu-latest'
config: 'Release'
cmake_generator: 'Unix Makefiles'
shared: 'OFF'
- os: 'ubuntu-latest'
config: 'Debug'
cmake_generator: 'Ninja'
shared: 'OFF'
- os: 'ubuntu-latest'
config: 'Release'
cmake_generator: 'Unix Makefiles'
shared: 'ON'
- os: 'macos-latest'
config: 'Release'
cmake_generator: 'Unix Makefiles'
shared: 'OFF'
- os: 'macos-latest'
config: 'Debug'
cmake_generator: 'Ninja'
shared: 'OFF'
- os: 'macos-latest'
config: 'Release'
cmake_generator: 'Ninja'
shared: 'ON'
runs-on: ${{ matrix.os }}
steps:
- name: 'Checkout repo'
uses: actions/checkout@v2
- name: 'Install meson (Ubuntu)'
if: ${{ contains(matrix.os, 'ubuntu') && (matrix.cmake_generator == 'Ninja') }}
run: |
sudo apt-get install ninja-build
- name: 'Install meson (Macos)'
if: ${{ contains(matrix.os, 'macos') && (matrix.cmake_generator == 'Ninja') }}
run: |
brew install meson
- name: 'CMake configure'
run: |
mkdir build
cd build
cmake .. -DBUILD_SHARED_LIBS=${{ matrix.shared }} -G "${{ matrix.cmake_generator }}" \
-DCMAKE_BUILD_TYPE=${{ matrix.config }} -DFLATCC_INSTALL=ON \
-DCMAKE_INSTALL_PREFIX=${{ github.workspace }}/prefix
- name: 'CMake build'
run: |
cmake --build build --parallel
- name: 'CTest'
run: |
cd build
ctest -V -C ${{ matrix.config }}
- name: 'Install'
run: |
cmake --build build --target install
- name: 'Test installed flatcc prefix'
run: |
mkdir user && cd user
cmake "${{ github.workspace }}/test/install_test" -DCMAKE_PREFIX_PATH="${{ github.workspace }}/prefix"
cmake --build .
ctest .
test_windows:
name: 'Test Windows with CMake'
strategy:
matrix:
config: ['Release', 'Debug']
arch: ['Win32', 'x64']
runs-on: 'windows-latest'
steps:
- name: 'Checkout repo'
uses: actions/checkout@v2
- name: 'CMake configure'
run: |
mkdir build
cd build
cmake .. -A ${{ matrix.arch }} -DFLATCC_INSTALL=ON -DCMAKE_INSTALL_PREFIX=${{ github.workspace }}/prefix
- name: 'CMake build'
run: |
cmake --build build --parallel --config ${{ matrix.config }} --verbose
- name: 'CTest'
run: |
cd build
ctest -V -C ${{ matrix.config }}
- name: 'Install'
run: |
cmake --build build --target install --config ${{ matrix.config }}
- name: 'Test installed flatcc prefix'
run: |
mkdir user && cd user
cmake "${{ github.workspace }}/test/install_test" -A ${{ matrix.arch }} `
-DCMAKE_PREFIX_PATH="${{ github.workspace }}/prefix"
cmake --build . --config ${{ matrix.config }}
ctest . -C ${{ matrix.config }}
test_mingw_on_linux:
name: 'Test cross building to Windows with CMake'
strategy:
matrix:
config: ['Release']
arch: ['x64']
shared_build: ['ON']
config_build: ['Release']
shared_host: ['ON']
config_host: ['Release']
runs-on: 'ubuntu-latest'
steps:
- name: 'Install mingw'
run: |
sudo apt-get install mingw-w64
- name: 'Checkout repo'
uses: actions/checkout@v2
- name: 'CMake configure/build/install build flatcc (native)'
run: |
mkdir build_native
cd build_native
cmake .. -DBUILD_SHARED_LIBS=${{ matrix.shared_build }} -DCMAKE_BUILD_TYPE=${{ matrix.config_build }} \
-DFLATCC_INSTALL=ON -DCMAKE_INSTALL_PREFIX=${{ github.workspace }}/prefix_build
cmake --build . --parallel
cmake --build . --target install
- name: 'CMake configure/build/install host flatcc (cross)'
run: |
mkdir build_mingw
cd build_mingw
cmake .. -DCMAKE_TOOLCHAIN_FILE=../test/cmake/x86_64-w64-mingw32-toolchain.cmake \
-DBUILD_SHARED_LIBS=${{ matrix.shared_host }} -DFLATCC_INSTALL=ON \
-DCMAKE_BUILD_TYPE=${{ matrix.config_host }} \
-DCMAKE_INSTALL_PREFIX=${{ github.workspace }}/prefix_host \
-DFLATCC_ROOT_FOR_BUILD=${{ github.workspace }}/prefix_build
cmake --build . --parallel
cmake --build . --target install
- name: 'Build a flatbuffers project with both native and host flatcc'
run: |
mkdir project
cd project
cmake ../test/install_test -DCMAKE_TOOLCHAIN_FILE=../test/cmake/x86_64-w64-mingw32-toolchain.cmake \
-DCMAKE_PREFIX_PATH=${{ github.workspace }}/prefix_host \
-DFLATCC_ROOT_FOR_BUILD=${{ github.workspace }}/prefix_build
cmake --build .
test_mingw_on_windows:
name: 'Test mingw on Windows'
strategy:
matrix:
shared: ['ON']
config: ['Release']
runs-on: 'windows-latest'
steps:
- name: 'Checkout repo'
uses: actions/checkout@v2
- name: 'CMake configure'
run: |
mkdir build
cd build
cmake .. -DBUILD_SHARED_LIBS=${{ matrix.shared }} -DCMAKE_BUILD_TYPE=${{ matrix.config }} `
-DFLATCC_INSTALL=ON -DCMAKE_INSTALL_PREFIX="${{ github.workspace }}/prefix" `
-G "MinGW Makefiles"
- name: 'CMake build'
run: |
cmake --build build --parallel
- name: 'CTest'
run: |
# FIXME: try using https://cmake.org/cmake/help/latest/prop_test/ENVIRONMENT.html on each test
$env:Path += ";${{ github.workspace }}/bin"
cd build
ctest -V -C ${{ matrix.config }}
- name: 'Install'
run: |
cmake --build build --target install
- name: 'Test installed flatcc prefix'
run: |
$env:Path += ";${{ github.workspace }}/prefix/bin"
mkdir user && cd user
cmake "${{ github.workspace }}/test/install_test" `
-DCMAKE_PREFIX_PATH="${{ github.workspace }}/prefix" -G "MinGW Makefiles"
cmake --build .
ctest .
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
build/*
bin/*
lib/*
lib64/*
release/*
scripts/build.cfg
Loading