-
Notifications
You must be signed in to change notification settings - Fork 164
ci: Teach CI to build C++03 deps #1054
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
base: main
Are you sure you want to change the base?
Conversation
477fc1e to
7aecf44
Compare
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.
Build 3167 of commit 7aecf44 has completed with FAILURE
7aecf44 to
0f019a2
Compare
PR is using c++03 deps cache entry even in non c++03 build, let me see what's up. |
Extra |
0f019a2 to
82b2fa4
Compare
4e21a4c to
4a58940
Compare
4a58940 to
176d05d
Compare
We key our cache entries for dependencies with the hash of the `build-deps.sh` script and the date the dependencies are built on. Because we run this job nightly, we end up with a new cache entry each night containing the same build results. This quickly fills up our cache with duplicate entries, crowding out space for the cache entries we use to communicate build results between jobs. In fact, it is very unusual to key a cache on the date in GitHub Actions. Most languages encourage you to use the hash of a dependency lock file (à la `yarn.lock`) for repeatability. In our case, the the `build-deps.sh` contains the BDE and NTF tags we need to build. As long as we use stable version tags or specific commit SHAs, each workflow run always tries to build the same source code, building the same binaries. This patch removes the date from the cache entry name. Signed-off-by: Patrick M. Niedzielski <[email protected]>
Signed-off-by: Patrick M. Niedzielski <[email protected]>
Signed-off-by: Patrick M. Niedzielski <[email protected]>
Signed-off-by: Patrick M. Niedzielski <[email protected]>
415a6d6 to
a3fb042
Compare
Signed-off-by: Patrick M. Niedzielski <[email protected]>
a3fb042 to
6e15dcb
Compare
Signed-off-by: Patrick M. Niedzielski <[email protected]>
|
@678098 you can take a look at this now. We're blocked on a PR to ntf-core for this, so we can't merge it anyway. I updated the PR description with some information. |
| working-directory: deps | ||
| run: ../docker/build_deps.sh | ||
| run: | | ||
| ${{ inputs.cpp03 }} ]] && ../docker/build_deps.sh --cpp03 || ../docker/build_deps.sh |
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.
| ${{ inputs.cpp03 }} ]] && ../docker/build_deps.sh --cpp03 || ../docker/build_deps.sh | |
| [[ ${{ inputs.cpp03 }} ]] && ../docker/build_deps.sh --cpp03 || ../docker/build_deps.sh |
Don't understand how it worked 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.
Hm not sure how it worked, but crucially we don't want [[ ... ]]. inputs.cpp03 expands to true or false, and both [[ true ]]; echo $? and [[ false ]]; echo $? evaluate to 0.
| required: false | ||
| type: string | ||
| default: "build-ubuntu" | ||
| cpp03: |
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.
What about adding a string property cxx_standard instead and explicitly pass cpp03 or cpp17 (or what version do we build here)?
This allows to normalize the code and remove pattern flag && A || B like [[ ${{ inputs.cpp03 }} ]] && CPP_VERSION_TAG="cpp03-" || CPP_VERSION_TAG=""
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.
Good idea, I'll go this route.
| -DCMAKE_BUILD_TYPE=Debug \ | ||
| -DBDE_BUILD_TARGET_SAFE=ON \ | ||
| -DBDE_BUILD_TARGET_64=ON \ | ||
| -DBDE_BUILD_TARGET_CPP03=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.
We don't provide configure script with ufid but can support something similar and simple like standard configuration if we update CMakeLists.txt
| on: | ||
| workflow_call: | ||
| inputs: | ||
| cpp03: |
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.
Can also have cxx_standard
| static_cast<bsls::Types::Int64>(port), | ||
| &localAllocator); | ||
| bsl::shared_ptr<bmqst::StatContext> portStatContext = | ||
| bslma::ManagedPtr<bmqst::StatContext> portStatContext = |
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.
Is this change required for cpp03?
Changing shared_ptr to ManagedPtr looks like a step back in this case.
Somehow we are able to build a shared_ptr later when we construct a PortContext
struct PortContext {
bsl::shared_ptr<bmqst::StatContext> d_portContext;
bsl::size_t d_numChannels;
};
This pull request teaches our CI to build bmqbrkr and bmqtool in C++03, which is a supported configuration for them.
Some notes about this PR:
configurescript will overwrite whatever UFID we pass it on Linux to compile with C++20. Feat: Let Linux/Darwin/Windows users pick their C++ version ntf-core#353 changes this behavior and is necessary for this to work.chore[DO NOT MERGE].shared_ptrthat seems to work fine on Solaris but not Linux+GCC.