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

Structure restructuring repo #28

Merged
merged 15 commits into from
Nov 26, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
32 changes: 29 additions & 3 deletions .github/workflows/msbuild.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ on: [push, pull_request]

env:
# Path to the solution file relative to the root of the project.
SOLUTION_FILE_PATH: .
SOLUTION_FILE_PATH: build\msw

# Configuration type to build.
# You can convert this to a build matrix if you need coverage of multiple configuration types.
Expand All @@ -25,8 +25,34 @@ jobs:
working-directory: ${{env.GITHUB_WORKSPACE}}
run: nuget restore ${{env.SOLUTION_FILE_PATH}}

- name: Build
- name: Build backend as a static library
working-directory: ${{env.GITHUB_WORKSPACE}}
# Add additional options to the MSBuild command line here (like platform or verbosity level).
# See https://docs.microsoft.com/visualstudio/msbuild/msbuild-command-line-reference
run: msbuild /m /p:Configuration=${{env.BUILD_CONFIGURATION}} ${{env.SOLUTION_FILE_PATH}}
run: msbuild /m /p:Configuration=${{env.BUILD_CONFIGURATION}} ${{env.SOLUTION_FILE_PATH}} /ignore:.vcxproj /t:VocalTractLabBackend

- name: Build backend tests
working-directory: ${{env.GITHUB_WORKSPACE}}
# Add additional options to the MSBuild command line here (like platform or verbosity level).
# See https://docs.microsoft.com/visualstudio/msbuild/msbuild-command-line-reference
run: msbuild /m /p:Configuration=${{env.BUILD_CONFIGURATION}} ${{env.SOLUTION_FILE_PATH}} /ignore:.vcxproj /t:VtlBackendTests

- name: Run backend tests
working-directory: ${{env.GITHUB_WORKSPACE}}
run: .\lib\Release\VtlBackendTests.exe

- name: Build API as a DLL
working-directory: ${{env.GITHUB_WORKSPACE}}
# Add additional options to the MSBuild command line here (like platform or verbosity level).
# See https://docs.microsoft.com/visualstudio/msbuild/msbuild-command-line-reference
run: msbuild /m /p:Configuration=${{env.BUILD_CONFIGURATION}} ${{env.SOLUTION_FILE_PATH}} /ignore:.vcxproj /t:VocalTractLabApi

- name: Build API tests
working-directory: ${{env.GITHUB_WORKSPACE}}
# Add additional options to the MSBuild command line here (like platform or verbosity level).
# See https://docs.microsoft.com/visualstudio/msbuild/msbuild-command-line-reference
run: msbuild /m /p:Configuration=${{env.BUILD_CONFIGURATION}} ${{env.SOLUTION_FILE_PATH}} /ignore:.vcxproj /t:VtlApiTests

- name: Run API tests
working-directory: ${{env.GITHUB_WORKSPACE}}
run: .\lib\Release\VtlApiTests.exe
63 changes: 60 additions & 3 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,9 +1,66 @@
# CMakeList.txt : CMake project for VocalTractLabApi, include source and define
# project specific logic here.
#
cmake_minimum_required (VERSION 3.8)
cmake_minimum_required (VERSION 3.13)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the a specific reason that makes this change possible?

Copy link
Contributor Author

@Simon-Stone Simon-Stone Nov 26, 2021

Choose a reason for hiding this comment

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

Looks good generally, but I do not like the way include/ is split up into Backend and VocalTractLabApi, since the source files are not split up either. In any way, a more fitting name for the api files would be API_C, as we may add a c++ api in the future. For the backend, I'd prefer lowercases, backend.

Splitting up the header files into Backend and VocalTractLabApi folders is best practice to avoid ambiguities. Let's say we were to use some other library at some point that included a header file with a name that already exists in the backend or API. If the header files were at the root level of include/, there was no way to unambiguously choose the right one to include. Using the subfolders, you just set the include/ directory and then include like so:

#include "Backend/Glottis.h"
#include "SomeOtherLib/Glottis.h

(This of course brings to attention the fact that we don't have namespaces in our codespace, but that is a topic for another time)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The uppercase Backend is conforming to Peter's conventions. As is the name VocalTractLabApi.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes, there was a reason why CMake 3.13 is required now, but I'm gonna have to check which routine made that necessary.

Copy link
Collaborator

@paul-krug paul-krug Nov 26, 2021

Choose a reason for hiding this comment

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

Splitting up the header files into Backend and VocalTractLabApi folders is best practice to avoid ambiguities. Let's say we were to use some other library at some point that included a header file with a name that already exists in the backend or API. If the header files were at the root level of include/, there was no way to unambiguously choose the right one to include. Using the subfolders, you just set the include/ directory and then include like so:

I get that, but why are the source files not split up, its the same situation isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would still be VocalTractLabBackend.dll or *.so, then.

Copy link
Contributor Author

@Simon-Stone Simon-Stone Nov 26, 2021

Choose a reason for hiding this comment

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

Hm, maybe we should change the name of the Backend folder(s) to VocalTractLabBackend? Seems more consistent compared to the API folders. And just Backend seems very generic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, maybe we should change the name of the Backend folder(s) to VocalTractLabBackend? Seems more consistent compared to the API folders. And just Backend seems very generic.

Yes, VocalTractLabBackend sounds better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would still be VocalTractLabBackend.dll or *.so, then.

True, you are right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I thought that we could simply wrap the VocalTractLabApi header around the static VocalTractLabBackend lib and build a shared lib from it, but apparently it is not as straight-forward. The recommended way of doing it is to include all the backend sources again explicitly when building the shared API library.

set(CMAKE_CXX_STANDARD 17)

project ("VocalTractLabApi")
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_SOURCE_DIR}/lib)
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_SOURCE_DIR}/lib)
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_SOURCE_DIR}/lib)

add_definitions(-D_USE_MATH_DEFINES -D_CRT_SECURE_NO_WARNINGS)
add_library (VocalTractLabApi SHARED VocalTractLabApi.def AnatomyParams.cpp AnatomyParams.h AudioFile.h Constants.h Dsp.cpp Dsp.h F0EstimatorYin.cpp F0EstimatorYin.h GeometricGlottis.cpp GeometricGlottis.h Geometry.cpp Geometry.h GesturalScore.cpp GesturalScore.h Glottis.cpp Glottis.h IirFilter.cpp IirFilter.h ImpulseExcitation.cpp ImpulseExcitation.h LfPulse.cpp LfPulse.h Matrix2x2.cpp Matrix2x2.h PoleZeroPlan.cpp PoleZeroPlan.h Sampa.cpp Sampa.h SegmentSequence.cpp SegmentSequence.h Signal.cpp Signal.h Splines.cpp Splines.h StaticPhone.cpp StaticPhone.h Surface.cpp Surface.h Synthesizer.cpp Synthesizer.h TdsModel.cpp TdsModel.h TimeFunction.cpp TimeFunction.h TlModel.cpp TlModel.h TriangularGlottis.cpp TriangularGlottis.h Tube.cpp Tube.h TubeSequence.h TwoMassModel.cpp TwoMassModel.h VocalTract.cpp VocalTract.h VocalTractLabApi.cpp VocalTractLabApi.h VoiceQualityEstimator.cpp VoiceQualityEstimator.h VowelLf.cpp VowelLf.h XmlHelper.cpp XmlHelper.h XmlNode.cpp XmlNode.h)

project("VocalTractLabApi")
add_library(VocalTractLabApi SHARED)
set_target_properties(VocalTractLabApi PROPERTIES PUBLIC_HEADER include/VocalTractLabApi/VocalTractLabApi.h)
target_include_directories (VocalTractLabApi PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/include/VocalTractLabApi)
target_include_directories (VocalTractLabApi PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include/Backend)
target_sources(VocalTractLabApi PRIVATE src/VocalTractLabApi.def src/AnatomyParams.cpp src/Dsp.cpp src/F0EstimatorYin.cpp src/GeometricGlottis.cpp src/Geometry.cpp src/GesturalScore.cpp src/Glottis.cpp src/IirFilter.cpp src/ImpulseExcitation.cpp src/LfPulse.cpp src/Matrix2x2.cpp src/PoleZeroPlan.cpp src/Sampa.cpp src/SegmentSequence.cpp src/Signal.cpp src/Splines.cpp src/StaticPhone.cpp src/Surface.cpp src/Synthesizer.cpp src/TdsModel.cpp src/TimeFunction.cpp src/TlModel.cpp src/TriangularGlottis.cpp src/Tube.cpp src/TwoMassModel.cpp src/VocalTract.cpp src/VocalTractLabApi.cpp src/VoiceQualityEstimator.cpp src/VowelLf.cpp src/XmlHelper.cpp src/XmlNode.cpp)


project("VocalTractLabBackend")
add_library(VocalTractLabBackend STATIC)
target_include_directories (VocalTractLabBackend PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/include/Backend)
target_sources(VocalTractLabBackend PUBLIC src/AnatomyParams.cpp src/Dsp.cpp src/F0EstimatorYin.cpp src/GeometricGlottis.cpp src/Geometry.cpp src/GesturalScore.cpp src/Glottis.cpp src/IirFilter.cpp src/ImpulseExcitation.cpp src/LfPulse.cpp src/Matrix2x2.cpp src/PoleZeroPlan.cpp src/Sampa.cpp src/SegmentSequence.cpp src/Signal.cpp src/Splines.cpp src/StaticPhone.cpp src/Surface.cpp src/Synthesizer.cpp src/TdsModel.cpp src/TimeFunction.cpp src/TlModel.cpp src/TriangularGlottis.cpp src/Tube.cpp src/TwoMassModel.cpp src/VocalTract.cpp src/VoiceQualityEstimator.cpp src/VowelLf.cpp src/XmlHelper.cpp src/XmlNode.cpp)


include(FetchContent)
FetchContent_Declare(
googletest
URL https://github.com/google/googletest/archive/609281088cfefc76f9d0ce82e1ff6c30cc3591e5.zip
)
# For Windows: Prevent overriding the parent project's compiler/linker settings
set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
FetchContent_MakeAvailable(googletest)

enable_testing()

add_executable(
"VtlApiTests"
"test/VtlApiTests.cpp"
)

target_include_directories (VtlApiTests PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/include/VocalTractLabApi)

target_link_libraries(
VtlApiTests
gtest_main
VocalTractLabApi
)

add_executable(
"VtlBackendTests"
"test/VtlBackendTests.cpp"
)

target_include_directories (VtlBackendTests PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/include)

target_link_libraries(
VtlBackendTests
gtest_main
VocalTractLabBackend
)

include(GoogleTest)
gtest_discover_tests(VtlApiTests WORKING_DIRECTORY ${CMAKE_SOURCE_DIR})
gtest_discover_tests(VtlBackendTests WORKING_DIRECTORY ${CMAKE_SOURCE_DIR})
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ cmake --build . --config Release
```

### Build using Visual Studio 2019 (Windows)
- Open ``VocalTractLabApi.sln``
- Open ``VocalTractLabApi.sln`` in the folder `build/msw`
- Build the project ``VocalTractLabApi``

6 changes: 0 additions & 6 deletions Unit Tests/pch.cpp

This file was deleted.

8 changes: 0 additions & 8 deletions Unit Tests/pch.h

This file was deleted.

39 changes: 0 additions & 39 deletions VocalTractLabApi.sln

This file was deleted.

Loading