Skip to content

FIXED: alternate fix for macOS static_assert issue#875

Merged
levb merged 2 commits into
fix_status_and_flapperfrom
lev-fix-static-assert-c99
Jun 9, 2025
Merged

FIXED: alternate fix for macOS static_assert issue#875
levb merged 2 commits into
fix_status_and_flapperfrom
lev-fix-static-assert-c99

Conversation

@levb

@levb levb commented Jun 9, 2025

Copy link
Copy Markdown
Collaborator

(looking into KeyValueDiscardOldToNew failure...)

@levb
levb force-pushed the lev-fix-static-assert-c99 branch from f01636a to 98703a5 Compare June 9, 2025 14:24

@kozlovic kozlovic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM - As long as it fixes the build on macOS, I am fine with this approach. For the failing test, I fixed it in the other PR (see code changes done in test.c for that failing test)

@levb
levb changed the base branch from main to fix_status_and_flapper June 9, 2025 14:27
@levb

levb commented Jun 9, 2025

Copy link
Copy Markdown
Collaborator Author

@kozlovic yes, I just rebased on your other PR to pull in that fix. I verified that this compiles on my MacOS in both C99 and C11 modes, and that the check fired in both. Waiting for the rebased version to pass...

@kozlovic kozlovic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why don't you submit the PR out of the main branch and simply add the fix to the test that is required because of the new server behavior? Or if you want, I can have a PR with just the test fix and close the PR about the compiler. Then you can rebase your PR from main with just the changes to assert?

@codecov

codecov Bot commented Jun 9, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.28%. Comparing base (3d53216) to head (391eaeb).
Report is 1 commits behind head on fix_status_and_flapper.

Additional details and impacted files
@@                    Coverage Diff                     @@
##           fix_status_and_flapper     #875      +/-   ##
==========================================================
- Coverage                   70.29%   70.28%   -0.01%     
==========================================================
  Files                          47       47              
  Lines                       15558    15558              
  Branches                     3206     3206              
==========================================================
- Hits                        10936    10935       -1     
- Misses                       1541     1544       +3     
+ Partials                     3081     3079       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@levb

levb commented Jun 9, 2025

Copy link
Copy Markdown
Collaborator Author

@kozlovic because I am proposing a change to your PR, so this is the right order? 1/5 I should've done it to begin with...

@levb
levb marked this pull request as ready for review June 9, 2025 14:33
@levb
levb requested a review from kozlovic June 9, 2025 14:35
@kozlovic

kozlovic commented Jun 9, 2025

Copy link
Copy Markdown
Member

because I am proposing a change to your PR, so it'd be the right order? 1/5 I should've done it to begin with...

@levb Oh, I would have done it directly from main (and simply take the change for the KV test). But I should have actually have the KV test fixed independently in the first place!

@kozlovic kozlovic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@levb
levb merged commit fde29bf into fix_status_and_flapper Jun 9, 2025
@levb
levb deleted the lev-fix-static-assert-c99 branch June 9, 2025 14:36
levb added a commit that referenced this pull request Jun 9, 2025
* Fixed compiler issue on macOS related to assert for status text

Made this a test instead. Also fixed a flapper that was due to a
recent change in the NATS Server where an INFO in response of the
CONNECT protocol is now sent, causing the override server version
that we use in the test to be replaced with the actual server
version.

Relates to #873

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

* FIXED: alternate fix for macOS static_assert issue (#875)

* FIXED: alternate fix for macOS static_assert issue

* Removed the new test

---------

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Co-authored-by: Lev <1187448+levb@users.noreply.github.com>
kozlovic added a commit that referenced this pull request Jul 28, 2025
* Fixed compiler issue on macOS related to assert for status text

Made this a test instead. Also fixed a flapper that was due to a
recent change in the NATS Server where an INFO in response of the
CONNECT protocol is now sent, causing the override server version
that we use in the test to be replaced with the actual server
version.

Relates to #873

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

* FIXED: alternate fix for macOS static_assert issue (#875)

* FIXED: alternate fix for macOS static_assert issue

* Removed the new test

---------

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Co-authored-by: Lev <1187448+levb@users.noreply.github.com>
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.

2 participants