Multi Platform Extensive Github Workflow Update#261
Conversation
nihalzp
requested review from
adisidev and
Copilot
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR adds a comprehensive multi-platform CI pipeline, removes an outdated Docker build workflow, and applies formatting and minor bug-fix tweaks across the codebase.
- Introduce GitHub Actions CI (
.github/workflows/ci.yml) for style checks, Linux/macOS builds, sanitizers, and unit/stress tests - Remove legacy Docker-based CI (
.github/workflows/build.yml) and disable IPO on Clang/Linux in CMake - Apply consistent line-wrapping, brace placement, and initialize previously uninitialized locals in several modules
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/ci.yml | New CI covering style, build matrix, sanitizers, and tests |
| .github/workflows/build.yml | Removed legacy Docker build workflow |
| CMakeLists.txt | Disable IPO on Clang/Linux to avoid linker issues |
| src/cartogram_info/read_geojson.cpp | Initialize erico to avoid undefined behavior |
| src/cartogram_info/write_geojson.cpp | Initialize inset_c_bb to a known invalid bounding box |
Multiple src/inset_state/*.cpp files |
Line-wrapping and brace-style consistency updates |
Various header files (*.hpp) |
Formatting cleanup (includes, function signatures, small fixes) |
Comments suppressed due to low confidence (2)
src/cartogram_info/read_geojson.cpp:76
- [nitpick] The variable name 'erico' is ambiguous. Consider renaming it to something more descriptive like 'is_exterior_ring_clockwise' to improve readability.
bool erico = false; // Exterior ring is clockwise oriented?
src/cartogram_info/write_geojson.cpp:69
- [nitpick] Using raw sentinel values for invalid bbox initialization can be error-prone. Consider providing a named factory (e.g.,
Bbox::invalid()) or a constant to encapsulate this intent.
Bbox inset_c_bb = Bbox(dbl_inf, dbl_inf, -dbl_inf, -dbl_inf); // Initialize to invalid bbox
adisidev
left a comment
adisidev
left a comment
There was a problem hiding this comment.
Looks mostly good, but I think we should still address two things:
- If we plan to retain the
Dockerfile, should we also retain the instructions? - Could we add more comments / explanation about the CI workflow so that new users can easily pick up where we leave off?
| For Windows users, we recommend using our program through Windows Subsystem for Linux (WSL). | ||
|
|
||
|
|
||
| #### Installing using Docker |
There was a problem hiding this comment.
If we're still retaining the Dockerfile, should we retain the instructions too?
There was a problem hiding this comment.
The Docker instructions were bit outdated. Additionally, having the Dockerfile is nice but I am in the support of later removing since we ensure that our program builds okay on all likely combination of supported compiler and OS. Anyone who can start Docker can also easily build the program by following the Linux commands.
What do you think? Should we keep the Dockerfile? If we keep, we might want to also have a CI that runs the Dockerfile and checks if the instructions are okay or not.
There was a problem hiding this comment.
Regardless, this can be part of a future PR as the integrating Docker build on the current CI is a big enough task.
There was a problem hiding this comment.
I agree, for now I think we should just remove the Dockerfile, and retrieve it from our commit history whenever required.
|
I have removed the Dockerfile, and will approve and merge the pull request now. |
The new workflow includes style checks, multi-platform multi-type builds (OS: Linux, macOS, Compiler: gcc, clang, apple-clang), sanitizer builds (AddressSanitizer, UndefinedBehaviorSanitizer, ThreadSanitizer), and separate jobs for unit and stress tests.