Skip to content
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 #1994 gorge, staticExec now raise Assert on exitCode !=0; simplified implementation #10345

Closed
wants to merge 1 commit into from

Conversation

timotheecour
Copy link
Member

Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@@ -67,7 +67,9 @@
- two poorly documented and not used modules (`subexes`, `scgi`) were moved to
graveyard (they are available as Nimble packages)


- `gorge`, `staticExec` now raise AssertError if exitCode is not 0 instead of
Copy link
Contributor

Choose a reason for hiding this comment

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

Assertions aren't catchable, I think we want something that is catchable to be raised.

Copy link
Member Author

@timotheecour timotheecour Jan 18, 2019

Choose a reason for hiding this comment

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

but that's at compile time; user can always use the cleaner gorgeEx variant which returns exitCode and handle accordingly so there's 0 loss of functionality.

In any case, catching doesn't work:

static:
  try:
    let a = gorge("D20190116T211842")
  except OSError as e:
    echo "caught"

gives:

Error: cannot evaluate at compile time: currException

Copy link
Contributor

@krux02 krux02 left a comment

Choose a reason for hiding this comment

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

You should not just break the compatibility with existing code that uses the existing API. Please find a deprecation path that doesn't break existing code.

tests/vm/tvmops.nim Show resolved Hide resolved
@krux02
Copy link
Contributor

krux02 commented Jan 17, 2019

one possible solution is like in bash, it the exit code always in a special variable, but that is bad in multithreaded environments.

You should rename the title of this review. With the title how it is now I wanted to close this PR without even looking at the code, because an assert would break my usages of staticExec.

@timotheecour
Copy link
Member Author

timotheecour commented Jan 18, 2019

You should not just break the compatibility with existing code that uses the existing API. Please find a deprecation path that doesn't break existing code.

discussed in #10360

one possible solution is like in bash, it the exit code always in a special variable, but that is bad in multithreaded environments.

that's too error prone. Note also that this is all compile time.

You should rename the title of this review. With the title how it is now I wanted to close this PR without even looking at the code, because an assert would break my usages of staticExec.

a PR shouldn't be closed just because there's 1 thing you dislike about it in the title, especially when that thing can be addressed/fixed during review.

If you have a concrete way to raise an exception that can be caught, please explain (after having tested it to work): as I've explained above, if I call raise newException(OSError) instead of calling doAssert, attempt at catching it results in Error: cannot evaluate at compile time: currException (see #10345 (comment))

  • I'm hitting a technical wall with this approach, which is the test failure for ./tests/vm/tgorge.nim gorgehas the odd (and undocumented) behavior of settingworkingDirtofooifgorgeis called fromfoo/bar.nim; I couldn't figure out how to keep that behavior with approach in this PR that makes gorgea regular proc callinggorgeEx, because: if gorgeis a regular proc, we have no knowledge (at CT) of caller; ifgorgeis made a template (and usinginstantiationInfo`), it also leads to other issues that I cannot solve (without changing the compiler).

because of that, I'm closing this in favor of #10360 but if someone has a concrete idea (and has tested it to work!) for how to make it work, please let me know and I'll reopen this PR as it's cleaner in some ways

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.

Can't tell return value of programs with staticExec
4 participants