-
Notifications
You must be signed in to change notification settings - Fork 558
[WIP] Ability to use Standard Library as a substitute for nostd:: classes #275
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
Changes from all commits
5ea2e9f
96fa468
9e398aa
1468119
9acb77c
6fb3a75
67a9f44
b003fc4
10f499e
f3b9eae
711de7a
f0427e5
4c1cd55
cb7326a
8fe51b1
f839760
830a502
1072ccf
9b185ba
7c8b159
5d036c5
a222d52
9ce018a
0e7c661
2557c35
b3a0c19
2c988c8
1f718a2
4eeaaee
3d30ff6
f79bf8d
0f1e3e5
bc7a3be
fe21b45
6270846
5b8966c
03bfd28
be2cdef
b6df1ca
d4e0e67
3788e79
42a0c93
416b543
410fc22
e273b0b
926d012
23f614d
277b8fa
9fbe2bb
ac62995
9ad55fb
406fbb0
7f0e450
b0777c0
c79d4f7
9702905
ac03ac1
36d0627
ebf7fea
5f98e6b
6ac3a6f
1ba4157
266cfe6
8f07150
15c38e0
72abaee
08076bb
e15c002
4bebaf1
79dc7b6
f94cf69
9f3cb7f
d921169
7bd0a0d
ccfd1c7
b857464
f7d6e28
131b2e6
9d7a693
9acefa5
302734a
d59c460
366f81d
ff0c1e0
6ece3ec
96dcbe2
8135f5e
fb83a19
187d800
3f66116
55eb3f7
657e4c8
6a33447
3bfc177
27cdca0
37f0a82
b931949
6fa1e11
464dcb8
124ad8b
77d0612
717c7ed
740dfb9
d9a435a
e4d2171
2f6cb5b
33af4af
a1ab6c7
dac6090
915ee19
769afdc
86a1eba
fe9d40e
d548ed4
cb8e437
6f24b21
85fa56b
3a22787
ebd0727
4cbdf9e
2fc1da0
420867f
611df39
fba85a4
0407173
17dc5e6
8dd6267
9761067
3cb7423
90466a7
6ac7f0e
2c8fbc5
34a7b0d
1c725c4
4ce4e97
4004c2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| name: C/C++ CI on Mac OS X | ||
|
|
||
| # Controls when the action will run. Triggers the workflow on push or pull request | ||
| # events but only for the master branch | ||
| on: | ||
| push: | ||
| branches: | ||
| - master | ||
| - build/* | ||
| pull_request: | ||
| branches: | ||
| - master | ||
| - build/* | ||
| schedule: | ||
| - cron: 0 2 * * 1-5 | ||
|
|
||
| # A workflow run is made up of one or more jobs that can run sequentially or in parallel | ||
| jobs: | ||
| build: | ||
| name: Build on ${{ matrix.os }} ${{ matrix.config }} | ||
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: | ||
| config: [release] | ||
| os: [macos-latest] | ||
| steps: | ||
|
|
||
| - name: Checkout | ||
| uses: actions/checkout@v2 | ||
| with: | ||
| submodules: 'true' | ||
|
|
||
| - name: Build | ||
| run: tools/build.sh ${{ matrix.config }} | ||
|
|
||
| - name: Test | ||
| run: ./tools/run-tests-all.sh |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| name: C/C++ CI on Ubuntu | ||
|
|
||
| # Controls when the action will run. Triggers the workflow on push or pull request | ||
| # events but only for the master branch | ||
| on: | ||
| push: | ||
| branches: | ||
| - master | ||
| - build/* | ||
| pull_request: | ||
| branches: | ||
| - master | ||
| - build/* | ||
| schedule: | ||
| - cron: 0 2 * * 1-5 | ||
|
|
||
| # A workflow run is made up of one or more jobs that can run sequentially or in parallel | ||
| jobs: | ||
| build: | ||
| name: Build on ${{ matrix.os }} ${{ matrix.config }} | ||
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: | ||
| config: [release] | ||
| os: [ubuntu-18.04, ubuntu-20.04] | ||
| steps: | ||
|
|
||
| - name: Checkout | ||
| uses: actions/checkout@v2 | ||
| with: | ||
| submodules: 'true' | ||
|
|
||
| - name: Build | ||
| run: tools/build.sh ${{ matrix.config }} | ||
|
|
||
| - name: Test | ||
| run: ./tools/run-tests-all.sh |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| name: C/C++ CI on Windows 2016 (vs2017) | ||
|
|
||
| # Controls when the action will run. Triggers the workflow on push or pull request | ||
| # events but only for the master branch | ||
| on: | ||
| push: | ||
| branches: | ||
| - master | ||
| - build/* | ||
| pull_request: | ||
| branches: | ||
| - master | ||
| - build/* | ||
| schedule: | ||
| - cron: 0 2 * * 1-5 | ||
|
|
||
| # A workflow run is made up of one or more jobs that can run sequentially or in parallel | ||
| jobs: | ||
| build: | ||
| name: Build on Windows ${{ matrix.arch }}-${{ matrix.config }} | ||
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: | ||
| arch: [x64] | ||
| config: [release] | ||
| os: [windows-2016] | ||
| steps: | ||
|
|
||
| - name: Checkout | ||
| uses: actions/checkout@v2 | ||
| with: | ||
| submodules: 'true' | ||
|
|
||
| - name: Setup build tools | ||
| run: | | ||
| cd "$Env:GITHUB_WORKSPACE" | ||
| .\tools\setup-buildtools.cmd | ||
|
|
||
| - name: Build | ||
| run: | | ||
| cd "$Env:GITHUB_WORKSPACE" | ||
| .\tools\build-vs2017-x64.cmd ${{ matrix.config }} | ||
|
|
||
| - name: Test | ||
| run: | | ||
| cd "$Env:GITHUB_WORKSPACE" | ||
| .\tools\run-tests-all.cmd |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| name: C/C++ CI on Windows 2019 (vs2019) | ||
|
|
||
| # Controls when the action will run. Triggers the workflow on push or pull request | ||
| # events but only for the master branch | ||
| on: | ||
| push: | ||
| branches: | ||
| - master | ||
| - build/* | ||
| pull_request: | ||
| branches: | ||
| - master | ||
| - build/* | ||
| schedule: | ||
| - cron: 0 2 * * 1-5 | ||
|
|
||
| # A workflow run is made up of one or more jobs that can run sequentially or in parallel | ||
| jobs: | ||
| build: | ||
| name: Build on Windows ${{ matrix.arch }}-${{ matrix.config }} | ||
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: | ||
| arch: [x64] | ||
| config: [release] | ||
| os: [windows-2019] | ||
| steps: | ||
|
|
||
| - name: Checkout | ||
| uses: actions/checkout@v2 | ||
| with: | ||
| submodules: 'true' | ||
|
|
||
| - name: Setup build tools | ||
| run: | | ||
| cd "$Env:GITHUB_WORKSPACE" | ||
| .\tools\setup-buildtools.cmd | ||
|
|
||
| - name: Build | ||
| run: | | ||
| cd "$Env:GITHUB_WORKSPACE" | ||
| .\tools\build-vs2019-x64.cmd ${{ matrix.config }} | ||
|
|
||
| - name: Test | ||
| run: | | ||
| cd "$Env:GITHUB_WORKSPACE" | ||
| .\tools\run-tests-all.cmd |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,22 @@ | ||
| [submodule "third_party/prometheus-cpp"] | ||
| path = third_party/prometheus-cpp | ||
| url = https://github.com/jupp0r/prometheus-cpp.git | ||
|
|
||
| [submodule "tools/vcpkg"] | ||
| path = tools/vcpkg | ||
| url = https://github.com/Microsoft/vcpkg | ||
| branch = master | ||
|
|
||
| [submodule "third_party/ms-gsl"] | ||
| path = "third_party/ms-gsl" | ||
| url = https://github.com/microsoft/GSL | ||
| branch = master | ||
|
|
||
| [submodule "third_party/googletest"] | ||
| path = third_party/googletest | ||
| url = https://github.com/google/googletest | ||
| branch = master | ||
|
|
||
| [submodule "third_party/benchmark"] | ||
| path = third_party/benchmark | ||
| url = https://github.com/google/benchmark | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,36 @@ if(NOT DEFINED CMAKE_CXX_STANDARD) | |
| set(CMAKE_CXX_STANDARD 11) | ||
| endif() | ||
|
|
||
| option(WITH_STL "Whether to use Standard Library for C++latest features" OFF) | ||
|
|
||
| option(WITH_ABSEIL "Whether to use Abseil for C++latest features" OFF) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: if both option are usable or not usable together, should we call that out?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jsuereth - This is complicated, but I'll try to elaborate :)
So - it's problematic on all accounts everywhere:
So -- to answer your question, in my setup I would prefer:
|
||
| if(WITH_ABSEIL) | ||
| add_definitions(-DHAVE_ABSEIL) | ||
| find_package(absl CONFIG REQUIRED) | ||
| set(CORE_RUNTIME_LIBS absl::any absl::base absl::bits absl::city) | ||
| # target_link_libraries(main PRIVATE absl::any absl::base absl::bits | ||
| # absl::city) | ||
| endif() | ||
|
|
||
| if(WITH_STL) | ||
| # We require at least C++17. C++20 is optimal to avoid gsl::span | ||
| add_definitions(-DHAVE_CPP_STDLIB -DHAVE_GSL) | ||
| # We ask for 20, but we may get C++17 | ||
| set(CMAKE_CXX_STANDARD 20) | ||
| # Guidelines Support Library path | ||
| set(GSL_DIR third_party/ms-gsl) | ||
| include_directories(${GSL_DIR}/include) | ||
| if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") | ||
| set(CMAKE_CXX_FLAGS_SPEED "/O2") | ||
| set(CMAKE_CXX_FLAGS | ||
| "${CMAKE_CXX_FLAGS} /Zc:__cplusplus ${CMAKE_CXX_FLAGS_SPEED}") | ||
| endif() | ||
| if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") | ||
| # set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++2a") | ||
| endif() | ||
| endif() | ||
|
|
||
| option(WITH_OTPROTOCOL | ||
| "Whether to include the OpenTelemetry Protocol in the SDK" OFF) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,3 +12,9 @@ install( | |
| if(BUILD_TESTING) | ||
| add_subdirectory(test) | ||
| endif() | ||
|
|
||
| if(WITH_STL) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: can we include ABSEIL here too? |
||
| message("Building with standard library types...") | ||
| else() | ||
| message("Building with nostd types...") | ||
| endif() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,16 +11,20 @@ OPENTELEMETRY_BEGIN_NAMESPACE | |
| namespace common | ||
| { | ||
| using AttributeValue = nostd::variant<bool, | ||
| int, | ||
| int32_t, | ||
| int64_t, | ||
| unsigned int, | ||
| uint32_t, | ||
| uint64_t, | ||
| double, | ||
| nostd::string_view, | ||
| const char *, | ||
| #ifdef HAVE_SPAN_BYTE | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this part of this PR or noise? If so, are there any concerns around this #ifidef causing compatibility issues with dynamic linking SDKs, or are we ONLY using this in a static-linked manner?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jsuereth - apologies, I'm gonna send another PR from a separate branch to minimize the 'noise'. My master branch contained changes that I needed for some other flows, e.g. byte array support is needed for my internal customer needs, which is logged as an open issue in OT spec (but not implemented yet). I'll try to answer your questions in the new PR. |
||
| nostd::span<const uint8_t>, // TODO: not part of OT spec yet | ||
| #endif | ||
| nostd::span<const bool>, | ||
| nostd::span<const int>, | ||
| nostd::span<const int32_t>, | ||
| nostd::span<const int64_t>, | ||
| nostd::span<const unsigned int>, | ||
| nostd::span<const uint32_t>, | ||
| nostd::span<const uint64_t>, | ||
| nostd::span<const double>, | ||
| nostd::span<const nostd::string_view>>; | ||
|
|
@@ -35,7 +39,9 @@ enum AttributeType | |
| TYPE_DOUBLE, | ||
| TYPE_STRING, | ||
| TYPE_CSTRING, | ||
| // TYPE_SPAN_BYTE, // TODO: not part of OT spec yet | ||
| #if HAVE_SPAN_BYTE | ||
| TYPE_SPAN_BYTE, // TODO: not part of OT spec yet | ||
| #endif | ||
| TYPE_SPAN_BOOL, | ||
| TYPE_SPAN_INT, | ||
| TYPE_SPAN_INT64, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| #pragma once | ||
|
|
||
| #include "opentelemetry/context/context.h" | ||
|
|
||
| OPENTELEMETRY_BEGIN_NAMESPACE | ||
| namespace context | ||
| { | ||
|
|
||
| // The Token object provides is returned when attaching objects to the | ||
| // RuntimeContext object and is associated with a context object, and | ||
| // can be provided to the RuntimeContext Detach method to remove the | ||
| // associated context from the RuntimeContext. | ||
| class Token | ||
| { | ||
| public: | ||
| bool operator==(const Context &other) noexcept { return context_ == other; } | ||
|
|
||
| // The ContextDetacher object automatically attempts to detach | ||
| // the Token when all copies of the Token are out of scope. | ||
| class ContextDetacher | ||
| { | ||
| public: | ||
| ContextDetacher(Context context) : context_(context) {} | ||
|
|
||
| ~ContextDetacher(); | ||
|
|
||
| private: | ||
| Context context_; | ||
| }; | ||
|
|
||
| Token() noexcept = default; | ||
|
|
||
| // A constructor that sets the token's Context object to the | ||
| // one that was passed in. | ||
| Token(Context context) | ||
| { | ||
| context_ = context; | ||
|
|
||
| detacher_ = nostd::shared_ptr<ContextDetacher>(new ContextDetacher(context_)); | ||
| }; | ||
|
|
||
| Context context_; | ||
|
|
||
| nostd::shared_ptr<ContextDetacher> detacher_; | ||
| }; | ||
|
|
||
| class IRuntimeContext | ||
| { | ||
| public: | ||
| // Provides a token with the passed in context | ||
| Token CreateToken(Context context) noexcept { return Token(context); }; | ||
|
|
||
| virtual Context InternalGetCurrent() noexcept = 0; | ||
|
|
||
| virtual Token InternalAttach(Context context) noexcept = 0; | ||
|
|
||
| virtual bool InternalDetach(Token &token) noexcept = 0; | ||
| }; | ||
|
|
||
| } // namespace context | ||
| OPENTELEMETRY_END_NAMESPACE |
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.
should this be picking a specific version of ms-GSL for those who opt-in to it?
it wasn't clear from the repo how folks understand what version they're using and pull in bug fixes.
additionally, if we want to guarantee any kind of ABI/API compat we probably need to know exactly which version is getting linked in when the flag is used.
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.
Right now it's pulling the latest. But since we use submodules - for CMake build we pin-point to specific commit ID. Technically the MS-GSL is only needed for older compilers without C++20 support, i.e. latest-latest Mac and latest gcc-9+ (as well as vs2019 with c++_latest flag) no longer require MS-GSL. As an alternative - I also want to add an option to build with ABSEIL instead of MS-GSL....
Re. pinning in Bazel - I am not sure how to do it. I wonder if we add Abseil, then test the combo of Bazel+Abseil, and CMake+GSL, that'd be covering the most needed build configurations for those who want to build statically. I can add CMake+Abseil build flavor.