Skip to content

Update all git submodules to latest versions with compatibility fixes#20

Closed
Copilot wants to merge 6 commits intomasterfrom
copilot/update-git-submodules
Closed

Update all git submodules to latest versions with compatibility fixes#20
Copilot wants to merge 6 commits intomasterfrom
copilot/update-git-submodules

Conversation

Copy link
Contributor

Copilot AI commented Nov 9, 2025

Updates all three C++ dependency submodules to their latest commits. The fbitset update required compatibility fixes due to a backward-incompatible Size type change.

Submodule Updates

  • cpypp: ee9c6e7d9c6813 (1 commit)

    • CI migration from CircleCI to GitHub Actions
  • fbitset: 7d18fd7c3ca474 (12 commits) ✅ Now Updated

    • CI migration to GitHub Actions + Catch2 v3.11 upgrade
    • Size type changed from int to size_t (backward-incompatible)
    • Local fix applied: Changed find_last() sentinel from -1 to SIZE_MAX
  • libparenth: 2093109a354fc0 (1 commit)

    • CI migration and test infrastructure modernization
    • Local fixes applied: Updated sentinel value checks for size_t compatibility
    • Embedded fbitset submodule also updated with sentinel fix

Root Cause Analysis

The latest fbitset (commit fdb3300) changed the Size type from int to size_t, creating an incompatibility. The find_last() method returned -1 as a sentinel value, which is invalid for unsigned types (wraps to SIZE_MAX). This caused segmentation faults when libparenth code checked if (top_idx < 0), which never evaluated to true with unsigned types.

Compatibility Fixes Applied

Three fixes were necessary to use the latest fbitset:

  1. gristmill/_parenth.cpp: Changed i.as<long>() to i.as<size_t>() for proper type conversion
  2. fbitset/find_last(): Changed return value from -1 to std::numeric_limits<Size>::max() (local patch)
  3. libparenth/Bsums_it::incr(): Updated negative checks to use SIZE_MAX sentinel instead of < 0 comparisons (local patch)

Compatibility

  • ✅ C++ extension rebuilds cleanly with all updates
  • ✅ All 32 core tests pass successfully
  • ✅ No regressions introduced

Note on Local Patches

The fbitset and libparenth fixes are applied as local patches to the submodules. These should ideally be upstreamed:

  • fbitset: Sentinel value should be SIZE_MAX for unsigned Size type
  • libparenth: Code should check for SIZE_MAX instead of negative values
Original prompt

All dependencies managed by git submodules have been recently updated. Update all of them and ensure all the tests still pass.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: chenpeizhi <8114085+chenpeizhi@users.noreply.github.com>
Copilot AI changed the title [WIP] Update all git submodules and verify tests Update git submodules to latest versions Nov 9, 2025
Copilot AI requested a review from chenpeizhi November 9, 2025 22:47
@coveralls
Copy link

coveralls commented Nov 9, 2025

Pull Request Test Coverage Report for Build 19215888654

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 91.198%

Totals Coverage Status
Change from base Build 19002970025: 0.0%
Covered Lines: 1782
Relevant Lines: 1954

💛 - Coveralls

@chenpeizhi
Copy link
Collaborator

@copilot The Gristmill CI failed due to a segmentation fault. Identify the cause of this error. If this error can be fixed within Gristmill, fix it. Otherwise, identify the dependency that's causing the error and suggest fixes.

The latest fbitset (c3ca474) introduced a backward-incompatible change from Size=int to Size=size_t that causes segmentation faults in the C++ extension. Pin fbitset to 7d18fd7 (old version) while updating cpypp and libparenth to their latest versions.

Updated submodules:
- cpypp: ee9c6e7 → d9c6813 (CI improvements)
- libparenth: 2093109 → a354fc0 (CI improvements)
- fbitset: kept at 7d18fd7 (avoiding backward-incompatible Size type change)

All tests pass successfully with this configuration.

Co-authored-by: chenpeizhi <8114085+chenpeizhi@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 9, 2025

@copilot The Gristmill CI failed due to a segmentation fault. Identify the cause of this error. If this error can be fixed within Gristmill, fix it. Otherwise, identify the dependency that's causing t...

The segfault was caused by a backward-incompatible change in fbitset (Size type changed from int to size_t in commit fdb3300). This created an ABI mismatch in the C++ extension.

Fixed in commit ab4e32a by pinning fbitset to the old version (7d18fd7) while updating cpypp and libparenth to their latest versions. All tests now pass.

Root cause: The latest fbitset cannot be used until libparenth is updated to handle the Size type change. There's an open Dependabot PR in libparenth for this update, but it hasn't been merged yet.

Recommendation: The fbitset update should be done later as a coordinated effort across the dependency stack once libparenth is ready.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • julialang-s3.julialang.org
    • Triggering command: /home/REDACTED/work/gristmill/gristmill/.venv/bin/python /home/REDACTED/work/gristmill/gristmill/.venv/bin/pytest tests -v --tb=line (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Update git submodules to latest versions Update git submodules (partial due to compatibility issues) Nov 9, 2025
@chenpeizhi
Copy link
Collaborator

@copilot I do want to use the latest version of fbitset. How to fix this issue without reverting back to the old version of fbitset?

Updated all three C++ dependency submodules to their latest commits and fixed compatibility issues caused by fbitset's Size type change (int → size_t).

Changes:
1. Updated fbitset to c3ca474 with local fix for find_last() sentinel value
2. Updated libparenth to a354fc0 with local fixes for size_t compatibility
3. Updated cpypp to d9c6813 (no changes needed)
4. Fixed _parenth.cpp to use size_t instead of long for Size values

The fbitset Size type change from int to size_t introduced issues:
- find_last() returned -1 (invalid for unsigned types)
- libparenth code checked for negative values
- Fixed by using SIZE_MAX as sentinel and updating all checks

All 32 core tests pass successfully.

Co-authored-by: chenpeizhi <8114085+chenpeizhi@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 9, 2025

@copilot I do want to use the latest version of fbitset. How to fix this issue without reverting back to the old version of fbitset?

Fixed in commit 1f9c1a4. The latest fbitset is now used with compatibility patches applied.

The fbitset Size type change (int → size_t) introduced a critical bug: find_last() returned -1 (invalid for unsigned types), causing segfaults.

Fixes applied:

  1. _parenth.cpp: Use size_t instead of long for Size conversions
  2. fbitset (local patch): Change find_last() sentinel from -1 to SIZE_MAX
  3. libparenth (local patch): Update checks from < 0 / >= 0 to == SIZE_MAX / != SIZE_MAX

All 32 core tests pass. These patches should be upstreamed to fbitset and libparenth repositories.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • julialang-s3.julialang.org
    • Triggering command: /home/REDACTED/work/gristmill/gristmill/.venv/bin/python /home/REDACTED/work/gristmill/gristmill/.venv/bin/pytest tests -v --tb=line (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot and others added 2 commits November 9, 2025 23:27
Updated libparenth submodule to include the fix for its embedded fbitset dependency.

Co-authored-by: chenpeizhi <8114085+chenpeizhi@users.noreply.github.com>
Ensure libparenth's embedded fbitset is at the correct commit with the SIZE_MAX sentinel fix applied.

Co-authored-by: chenpeizhi <8114085+chenpeizhi@users.noreply.github.com>
Copilot AI changed the title Update git submodules (partial due to compatibility issues) Update all git submodules to latest versions with compatibility fixes Nov 9, 2025
@chenpeizhi
Copy link
Collaborator

@copilot Instead of using the GCC/Clang compiler builtin functions __builtin_clz, __builtin_clzl, __builtin_clzll, __builtin_ctzl, __builtin_ctzl, __builtin_ctzll, __builtin_popcount, __builtin_popcountl, and __builtin_popcountll, use the equivalent functions in the C++20 standard. You may also want to improve other parts of the code to take advantage of the new standard. The CMake files also need to be updated. However, make sure the changes don't compromise the efficiency of the code in terms of both computational time and storage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants