New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #10273 execShellCmd now returns nonzero when child killed with signal + other fixes #10274

Merged
merged 5 commits into from Jan 13, 2019

Conversation

Projects
None yet
2 participants
@timotheecour
Copy link
Collaborator

timotheecour commented Jan 11, 2019

  • fix #10273 : execShellCmd now returns nonzero (instead of 0) when child killed with signal
    • this is a serious bug as ignoring errors can have bad consequences
    • this could happen when child dies because of SIGSEGV, eg due to infinite recursion causing stack overflow
    • execShellCmd(cmd) now returns the same exit code as if you ran cmd in your shell, instead of 0
  • add tests for this in tests/stdlib/tosproc.nim
  • rename exitStatus() to exitStatusLikeShell() to make its meaning clear and distinguish from exitStatus variables which aren't necessarily referring to shell exit status
  • fix minor bug in .gitignore
  • properly fixes this #10231 (comment):

Looks like POSIX compat is a mess on Darwin. I guess this could just be fixed with a special when branch.

  • properly fix: #10231

  • adds a test case for #10249 and explanation for the bug: megatest compilation failure gives no useful information, hence hard to investigate

  • add lib/std/special_paths.nim

    • makes it easy for files (eg in testament) to refer to specific dirs/files, which are guaranteed to be correct at compile time (and don't depend on environment eg PATH)
    • refs #10268 (=> nim-lang/RFCs#119) by allowing testament tests to simply call: let output = buildDir / "D20190111T024543".addFileExt(ExeExt) ; tests/stdlib/tosproc.nim has an example of that;

note

  • the bug was pre-existant to #10222 so dates back since probably forever /cc @alaviss
    c_system(command) shr 8 was wrong, as was WEXITSTATUS(c_system(command)) since these could return 0 even when child exited with a signal that killed it (eg SIGSEGV)

@timotheecour timotheecour requested a review from Araq Jan 11, 2019

@timotheecour timotheecour changed the title fix #10273 execShellCmd now returns nonzero when child killed with signal fix #10273 execShellCmd now returns nonzero when child killed with signal + other fixes Jan 11, 2019

timotheecour referenced this pull request in alaviss/Nim Jan 11, 2019

os.execShellCmd: fixes nim-lang#10231
Darwin has long deprecated the wait union, but their macros still assume
it unless you define _POSIX_C_SOURCE. This trips up C++ compilers.

This commit duplicates the behavior of WEXITSTATUS when _POSIX_C_SOURCE
is defined.

@timotheecour timotheecour added the osproc label Jan 11, 2019

@timotheecour timotheecour force-pushed the timotheecour:pr_fix_exitStatus branch from fd4fec0 to b3c736e Jan 11, 2019

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Jan 11, 2019

  1. Why would I review this when it doesn't even bootstrap on Windows.
  2. Please avoid Posix macros, reimplement them, otherwise nlvm has a hard time keeping up. Can be done in a separate PR though.

@timotheecour timotheecour force-pushed the timotheecour:pr_fix_exitStatus branch 3 times, most recently from e4b3c8a to 9af042a Jan 11, 2019

@timotheecour

This comment has been minimized.

Copy link
Collaborator

timotheecour commented Jan 12, 2019

/cc @Araq
PTAL, green

Why would I review this when it doesn't even bootstrap on Windows.

Please avoid Posix macros, reimplement them, otherwise nlvm has a hard time keeping up. Can be done in a separate PR though.

definitely would belong to a separate PR given the macros were already being used in code before PR; care would have to be taken that platform specific behavior is preserved; more generally, there may be a way for nlvm that doesn't involve duplicating implementation of posix functions (need separate thread to discuss that)

@@ -0,0 +1,29 @@
#[

This comment has been minimized.

@Araq

Araq Jan 12, 2019

Member

This should go into tests/stdlib/specialpaths.nim.

This comment has been minimized.

@timotheecour

timotheecour Jan 12, 2019

Collaborator

moved it out of lib/std and put it here: testament/lib/stdtest/specialpaths.nim along with tests/nim.cfg that sets --path:"../testament/lib" so that:

  • testament tests can simply import stdtest/foo without messing with error prone relative paths ( "../../foo/bar")
  • there is 0 confusion possible with stdlib modolues

moving it to tests/stdlib/specialpaths.nim and adding tests/stdlib to path would not be good as it'd bring in package scope things that aren't meant to be imported.
Other general purpose helper modules that don't belong in stdlib but are useful for testament tests can go also under testament/lib/stdtest/

Show resolved Hide resolved lib/std/special_paths.nim Outdated

@timotheecour timotheecour force-pushed the timotheecour:pr_fix_exitStatus branch from 9af042a to 14e1e6f Jan 12, 2019

@timotheecour

This comment has been minimized.

Copy link
Collaborator

timotheecour commented Jan 12, 2019

@Araq PTAL

@Araq Araq merged commit 9af85fb into nim-lang:devel Jan 13, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Araq

This comment has been minimized.

Copy link
Member

Araq commented Jan 13, 2019

Note: os.nim now exports a new undocumented proc.

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