Skip to content

Conversation

@Qard
Copy link
Contributor

@Qard Qard commented Jul 10, 2025

Description

Depends on #488.

I've pulled the test for that out as it had a problematic interaction with the closure feature. I'll have to figure out how to make that work properly. 🤔

Checklist

Check the boxes that apply (put an x in the brackets, like [x]). You can also check boxes after the PR is created.

❤️ Thank you for your contribution!

@Qard Qard mentioned this pull request Jul 10, 2025
4 tasks
@Qard Qard force-pushed the add-sapi-zts-test branch 2 times, most recently from e7b5d98 to d7e34a9 Compare July 11, 2025 09:16
@ptondereau
Copy link
Member

Hello @Qard
We think your PR is a good addition.
Could it be possible to rebase here?

@Qard
Copy link
Contributor Author

Qard commented Oct 29, 2025

Yep, I can try a rebase. Though I never did get around to looking into the closure interaction. It may still be broken?

@Qard Qard force-pushed the add-sapi-zts-test branch 2 times, most recently from 957310a to 308f3c5 Compare October 29, 2025 21:25
@Qard
Copy link
Contributor Author

Qard commented Oct 29, 2025

I've pushed a possible fix to the closure conflict, though I'm uncertain if that is actually correct. Anyone else know the closure subsystem better that can review that?

@Qard Qard force-pushed the add-sapi-zts-test branch from 974432f to e21a272 Compare October 29, 2025 22:52
@Qard Qard force-pushed the add-sapi-zts-test branch from e21a272 to eaa9c69 Compare October 29, 2025 23:11
@ptondereau
Copy link
Member

I've pushed a possible fix to the closure conflict, though I'm uncertain if that is actually correct. Anyone else know the closure subsystem better that can review that?

I'll try to have a look next week. Thanks for the work

@Qard
Copy link
Contributor Author

Qard commented Oct 31, 2025

I've pushed a possible fix to the closure conflict, though I'm uncertain if that is actually correct. Anyone else know the closure subsystem better that can review that?

I'll try to have a look next week. Thanks for the work

It seemed to get past the original failure, but now fails in a different. Unfortunately though it seems to only fail in CI for me and not locally, so I'm not too sure how to debug. If I get time at some point I might take another look, but I'm not too sure how much sense I'd be able to make of it. 🤔

@ptondereau
Copy link
Member

I've pushed a possible fix to the closure conflict, though I'm uncertain if that is actually correct. Anyone else know the closure subsystem better that can review that?

I'll try to have a look next week. Thanks for the work

It seemed to get past the original failure, but now fails in a different. Unfortunately though it seems to only fail in CI for me and not locally, so I'm not too sure how to debug. If I get time at some point I might take another look, but I'm not too sure how much sense I'd be able to make of it. 🤔

Embed test runs only on 8.4 (embed sapi) debug nts with latest rust and llvm 17.

@ptondereau
Copy link
Member

ptondereau commented Nov 1, 2025

Ok, this is because I guess you have a ZTS build of PHP locally, and the Test embed runs only on a non-TS version.
This means that you probably need to add a test guard for ZTS build (we already have a #[cfg(php_zts)]) and add a matrix entry for test embed with PHP ZTS/NTS.

@Qard
Copy link
Contributor Author

Qard commented Nov 3, 2025

Where exactly? I'm not clear exactly what you're suggesting I change. 🤔

@coveralls
Copy link

coveralls commented Nov 3, 2025

Pull Request Test Coverage Report for Build 19052894881

Details

  • 1 of 2 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 31.091%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/closure.rs 1 2 50.0%
Totals Coverage Status
Change from base Build 18953724839: -0.007%
Covered Lines: 1359
Relevant Lines: 4371

💛 - Coveralls

@Qard Qard force-pushed the add-sapi-zts-test branch 2 times, most recently from edbe06f to d966aa4 Compare November 3, 2025 23:38
@Qard Qard force-pushed the add-sapi-zts-test branch from d966aa4 to 8ad3645 Compare November 3, 2025 23:44
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