fix partially #13115 (now works for cpp; but still fails for js on openbsd) #16167

timotheecour · 2020-11-28T04:38:27Z

fix partially #13115 (js on openbsd still broken for some reason)

/cc @xflywind
before PR: works for js,cpp,vm
after PR: also cpp

future work

doesn't yet work for openbsd + js ; EDIT: ... due to an upstream nodejs bug that has been patched in more recent nodejs version

euantorano · 2020-12-01T16:01:38Z

This seems to have a conflict with tests/exception/t13115.nim. I assume JS on OpenBSD still exhibits the same behaviour even with this patch? If so, that's quite strange and I'll need to dig a bit to work out why.

timotheecour · 2020-12-01T18:33:18Z

This seems to have a conflict with tests/exception/t13115.nim.

fixed

I assume JS on OpenBSD still exhibits the same behaviour even with this patch? If so, that's quite strange and I'll need to dig a bit to work out why.

thanks

timotheecour · 2020-12-03T18:18:49Z

friendly ping @Clyybber

Clyybber · 2020-12-03T18:50:13Z

I think this should work on OpenBSD before we merge this.

timotheecour · 2020-12-03T19:50:01Z

PTAL

I think this should work on OpenBSD before we merge this.

openbsd on js was already failing prior to this PR and fixing it can be done later. I believe this is an upstream nodejs bug similar to the one I had submitted last year (backtrace truncated if msg contains null character \0 nodejs/node#28761); we should report this upstream after confirming (/cc @euantorano since you have access to openbsd; can you please test the snippet in backtrace truncated if msg contains null character \0 nodejs/node#28761 on openbsd?).
I've amended PR msg and commit msg with s/fix/fix partially/ so newException terminates msg early from NULL byte, other IO does not #13115 won't auto-close when this PR is merged
I've improved test coverage for this bug by calling execCmdEx directly instead of relying on testament spec DSL which isn't suitable here; the test is stronger now:
- it now tests openbsd (except for js)
- it now checks the msg is exactly what we expect (including the \0)
- it now checks VM too

euantorano · 2020-12-03T19:54:57Z

It looks like there was a node patch to support the null character, I need to work out which versions that patch landed in as it’s possible that version isn’t packaged for OpenBSD yet. EDIT: looks like 12.16.1: https://openports.se/lang/node It looks like the node change was released in 12.16.2: nodejs/node#32313 It’s probably best to leave the test disabled for JS on OpenBSD for now if that’s the case, or else we may need to build our own copy of node as part of the build (or install a binary that’s not in ports...).

…

On Thu, 3 Dec 2020, at 19:50, Timothee Cour wrote: PTAL > I think this should work on OpenBSD before we merge this. * openbsd on js was already failing prior to this PR and fixing it can be done later. I believe this is an upstream nodejs bug similar to the one I had submitted last year (nodejs/node#28761 <nodejs/node#28761>); we should report this upstream after confirming (/cc @euantorano <https://github.com/euantorano> since you have access to openbsd). * I've amended PR msg and commit msg with `s/fix/fix partially/` so #13115 <#13115> won't auto-close when this PR is merged * I've improved test coverage for this bug by calling `execCmdEx` directly instead of relying on testament spec DSL; the test is stronger now: * it now tests openbsd (except for js) * it now checks the msg is exactly what we expect (including the `\0`) * it now checks VM too — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16167 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFW24J6ZRBD6GH5SG2GPXTSS7TXRANCNFSM4UFRMSPA>.

Clyybber · 2020-12-03T22:45:50Z

tests/exception/t13115.nim

+        let (outp, exitCode) = execCmdEx(cmd)
+        doAssert msg in outp, cmd & "\n" & msg
+        doAssert exitCode == 1
+  main()


Please get rid of this abomination and use testament to accomplish what you want.

testament spec DSL isn't flexible enough for this test, sorry. If you disagree, please show a complete example that tests VM, openbsd (for non-js case), the '\0' being part of the output message, allows avoiding combinatorial explosion (as i do in this PR) etc, while also avoiding generating N different test files (not DRY nor desirable).

testament spec may be good for the general case, but you'll always have special cases, which need things like:

trunner.nim

trunner_thirdparty.nim (remove .github/workflows/ci_ssl.yml; instead run via trunner_thirdparty #16221)

tosproc.nim

this test, etc

tests/exception/t13115.nim

…ails for js on openbsd)

timotheecour · 2020-12-11T00:57:18Z

friendly ping

…js on openbsd) (nim-lang#16167) * fix partially nim-lang#13115 properly (works for c,js,cpp,vm; still fails for js on openbsd) * address comment: also test with -d:danger, -d:debug

VlkrS · 2022-03-05T12:02:27Z

Both OpenBSD-current and OpenBSD-stable have a more recent version of nodejs now.
Is it possible to re-evaluate?

euantorano · 2022-03-05T14:50:50Z

Both OpenBSD-current and OpenBSD-stable have a more recent version of nodejs now. Is it possible to re-evaluate?

Certainly would be a good idea. Looks like the current version is 16.14.0: https://openports.se/lang/node

VlkrS · 2022-03-05T16:46:50Z

Certainly would be a good idea. Looks like the current version is 16.14.0: https://openports.se/lang/node

It's 12.22.9 for -stable, but in any case the fix from 12.16.2 should be in ;)

timotheecour mentioned this pull request Nov 28, 2020

newException terminates msg early from NULL byte, other IO does not #13115

Closed

timotheecour changed the title ~~fix #13115 properly (works for c,js,cpp,vm)~~ finish fix #13115 (now also works for cpp) Nov 28, 2020

timotheecour marked this pull request as draft November 28, 2020 05:14

timotheecour mentioned this pull request Nov 28, 2020

Disabled tests for OpenBSD #14035

Open

16 tasks

timotheecour changed the title ~~finish fix #13115 (now also works for cpp)~~ fix partially #13115 (now also works for cpp) Nov 28, 2020

timotheecour changed the title ~~fix partially #13115 (now also works for cpp)~~ fix partially #13115 (now also works for cpp) (partial because fails on openbsd) Nov 28, 2020

timotheecour marked this pull request as ready for review November 28, 2020 08:01

timotheecour requested a review from Clyybber November 29, 2020 00:12

timotheecour changed the title ~~fix partially #13115 (now also works for cpp) (partial because fails on openbsd)~~ fix #13115 (now also works for cpp) (but still fails on openbsd) Nov 29, 2020

timotheecour force-pushed the pr_fix_13115_cpp branch from 3ec9b19 to 2d28392 Compare December 1, 2020 18:31

timotheecour force-pushed the pr_fix_13115_cpp branch from 2d28392 to aafbc7c Compare December 2, 2020 00:25

timotheecour requested a review from ringabout December 3, 2020 18:17

timotheecour force-pushed the pr_fix_13115_cpp branch from aafbc7c to 76e258b Compare December 3, 2020 19:39

timotheecour changed the title ~~fix #13115 (now also works for cpp) (but still fails on openbsd)~~ fix partially #13115 (now also works for cpp) (but still fails on openbsd) Dec 3, 2020

timotheecour changed the title ~~fix partially #13115 (now also works for cpp) (but still fails on openbsd)~~ fix partially #13115 (now works for cpp; but still fails for js on openbsd) Dec 3, 2020

timotheecour force-pushed the pr_fix_13115_cpp branch 2 times, most recently from da0582d to f6893f2 Compare December 3, 2020 19:42

Clyybber reviewed Dec 3, 2020

View reviewed changes

timotheecour force-pushed the pr_fix_13115_cpp branch from f6893f2 to 41f552c Compare December 4, 2020 01:39

ringabout reviewed Dec 4, 2020

View reviewed changes

tests/exception/t13115.nim Show resolved Hide resolved

timotheecour added 4 commits December 4, 2020 14:33

fix partially nim-lang#13115 properly (works for c,js,cpp,vm; still f…

1cceff6

…ails for js on openbsd)

disbled: openbsd (fails for js)

0765ed7

improve test coverage

f623ea1

fix for windows

38b8e0e

timotheecour added 2 commits December 4, 2020 14:33

fixup

7f83b11

address comment: also test with -d:danger, -d:debug

d394231

timotheecour force-pushed the pr_fix_13115_cpp branch from d5ea1cd to d394231 Compare December 4, 2020 22:33

Araq merged commit bb1c962 into nim-lang:devel Dec 11, 2020

timotheecour deleted the pr_fix_13115_cpp branch December 11, 2020 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix partially #13115 (now works for cpp; but still fails for js on openbsd) #16167

fix partially #13115 (now works for cpp; but still fails for js on openbsd) #16167

timotheecour commented Nov 28, 2020 •

edited

euantorano commented Dec 1, 2020

timotheecour commented Dec 1, 2020

timotheecour commented Dec 3, 2020

Clyybber commented Dec 3, 2020

timotheecour commented Dec 3, 2020 •

edited

euantorano commented Dec 3, 2020 via email •

edited

Clyybber Dec 3, 2020 •

edited

timotheecour Dec 4, 2020 •

edited

timotheecour commented Dec 11, 2020

VlkrS commented Mar 5, 2022

euantorano commented Mar 5, 2022

VlkrS commented Mar 5, 2022

fix partially #13115 (now works for cpp; but still fails for js on openbsd) #16167

fix partially #13115 (now works for cpp; but still fails for js on openbsd) #16167

Conversation

timotheecour commented Nov 28, 2020 • edited

future work

euantorano commented Dec 1, 2020

timotheecour commented Dec 1, 2020

timotheecour commented Dec 3, 2020

Clyybber commented Dec 3, 2020

timotheecour commented Dec 3, 2020 • edited

euantorano commented Dec 3, 2020 via email • edited

Clyybber Dec 3, 2020 • edited

Choose a reason for hiding this comment

timotheecour Dec 4, 2020 • edited

Choose a reason for hiding this comment

timotheecour commented Dec 11, 2020

VlkrS commented Mar 5, 2022

euantorano commented Mar 5, 2022

VlkrS commented Mar 5, 2022

timotheecour commented Nov 28, 2020 •

edited

timotheecour commented Dec 3, 2020 •

edited

euantorano commented Dec 3, 2020 via email •

edited

Clyybber Dec 3, 2020 •

edited

timotheecour Dec 4, 2020 •

edited