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: Suppress subprocess traceback in case XONSH_SHOW_TRACEBACK=False and $RAISE_SUBPROC_ERROR=True #5066

Merged
merged 6 commits into from
Feb 27, 2023

Conversation

xmnlab
Copy link
Contributor

@xmnlab xmnlab commented Feb 20, 2023

Don't show the traceback for RAISE_SUBPROC_ERROR when XONSH_SHOW_TRACEBACK is false.

#4708

For community

⬇️ Please click the 👍 reaction instead of leaving a +1 or 👍 comment

@xmnlab
Copy link
Contributor Author

xmnlab commented Feb 20, 2023

This PR aims to propose an initial implementation for hide the traceback when using RAISE_SUBPROC_ERROR, this is the first time I am opening a PR for this project, so I am not super familiar with that, any recommendation to handle this in a better would be very appreciated. thanks!

@anki-code
Copy link
Member

anki-code commented Feb 20, 2023

Hello and welcome!
Be carefully because after you disabled the raising the exception the code after the error line will be executed but it should not because $RAISE_SUBPROC_ERROR = True. I think this is not your goal :)

echo @("""
$XONSH_SHOW_TRACEBACK = False
$RAISE_SUBPROC_ERROR = True

print('LINE 1')
cp nonexisting_file.txt other_name.txt
print("LINE 2")
""") > /tmp/1.xsh

xonsh /tmp/1.xsh
# Here should be only "LINE 1" and the short version of error.

@xmnlab
Copy link
Contributor Author

xmnlab commented Feb 20, 2023

thanks for the review @anki-code , I will fix that! thanks!

@xmnlab
Copy link
Contributor Author

xmnlab commented Feb 20, 2023

@anki-code with traceback disable

xonsh /tmp/test.xsh 
LINE 1
cp: cannot stat 'nonexisting_file.txt': No such file or directory

with traceback active

xonsh /tmp/test.xsh 
LINE 1
cp: cannot stat 'nonexisting_file.txt': No such file or directory
Traceback (most recent call last):
  File "/tmp/test.xsh", line 5, in <module>
    cp nonexisting_file.txt other_name.txt
    
  File "/mnt/sda1/storage/dev/xonsh-project/xonsh/xonsh/built_ins.py", line 205, in subproc_captured_hiddenobject
    return xonsh.procs.specs.run_subproc(cmds, captured="hiddenobject", envs=envs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/mnt/sda1/storage/dev/xonsh-project/xonsh/xonsh/procs/specs.py", line 903, in run_subproc
    return _run_specs(specs, cmds)
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/mnt/sda1/storage/dev/xonsh-project/xonsh/xonsh/procs/specs.py", line 936, in _run_specs
    command.end()
  File "/mnt/sda1/storage/dev/xonsh-project/xonsh/xonsh/procs/pipelines.py", line 458, in end
    self._end(tee_output=tee_output)
  File "/mnt/sda1/storage/dev/xonsh-project/xonsh/xonsh/procs/pipelines.py", line 477, in _end
    self._raise_subproc_error()
  File "/mnt/sda1/storage/dev/xonsh-project/xonsh/xonsh/procs/pipelines.py", line 605, in _raise_subproc_error
    raise subprocess.CalledProcessError(rtn, spec.args, output=self.output)
subprocess.CalledProcessError: Command '['cp', 'nonexisting_file.txt', 'other_name.txt']' returned non-zero exit status 1.

btw, I am using here sys.exit .. is there another way?

@anki-code anki-code marked this pull request as draft February 20, 2023 17:27
@anki-code anki-code changed the title fix: Add and extra condition for print traceback from subprocess fix: Suppress subprocess traceback in case XONSH_SHOW_TRACEBACK=False Feb 20, 2023
@anki-code anki-code changed the title fix: Suppress subprocess traceback in case XONSH_SHOW_TRACEBACK=False fix: Suppress subprocess traceback in case XONSH_SHOW_TRACEBACK=False and $RAISE_SUBPROC_ERROR=True Feb 20, 2023
@xmnlab
Copy link
Contributor Author

xmnlab commented Feb 21, 2023

tomorrow I will fix/update the tests.

@xmnlab xmnlab force-pushed the hide-traceback-for-RAISE_SUBPROC_ERROR branch from 1e93fe3 to af95a21 Compare February 24, 2023 19:25
@xmnlab
Copy link
Contributor Author

xmnlab commented Feb 24, 2023

@anki-code I was able to remove the sys.exit .. but not sure if that is a good alternative ...
any feedback would be very appreciated

btw, the pre-commit.ci erros look not related to this PR .. but if I am wrong, I would be happy to fix that.

@xmnlab xmnlab marked this pull request as ready for review February 27, 2023 17:05
@xmnlab
Copy link
Contributor Author

xmnlab commented Feb 27, 2023

hey everyone! I think this PR is ready for review .. would appreciate any suggestion to improve this PR :) thanks!

@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2023

Codecov Report

Merging #5066 (af95a21) into main (b600c58) will increase coverage by 0.53%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main    #5066      +/-   ##
==========================================
+ Coverage   66.23%   66.77%   +0.53%     
==========================================
  Files         129      129              
  Lines       23829    23831       +2     
  Branches     4500     4501       +1     
==========================================
+ Hits        15784    15912     +128     
+ Misses       6800     6691     -109     
+ Partials     1245     1228      -17     
Flag Coverage Δ
macOS-latest 64.08% <85.71%> (+0.01%) ⬆️
ubuntu-latest 64.35% <85.71%> (?)
windows-latest 62.76% <20.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
xonsh/main.py 83.38% <50.00%> (+1.75%) ⬆️
xonsh/procs/pipelines.py 80.78% <100.00%> (+0.43%) ⬆️
xonsh/procs/specs.py 73.59% <0.00%> (+0.34%) ⬆️
xonsh/environ.py 82.70% <0.00%> (+0.47%) ⬆️
xonsh/jobs.py 48.93% <0.00%> (+0.60%) ⬆️
xonsh/tools.py 72.56% <0.00%> (+0.63%) ⬆️
xonsh/xonfig.py 31.86% <0.00%> (+0.82%) ⬆️
xonsh/aliases.py 62.82% <0.00%> (+1.30%) ⬆️
xonsh/execer.py 78.57% <0.00%> (+1.78%) ⬆️
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jnoortheen
Copy link
Member

@xmnlab the changes look good. Could you fix the failing flake8 checks ?

@xmnlab
Copy link
Contributor Author

xmnlab commented Feb 27, 2023

thanks for the review @jnoortheen .. I can take a look if there is anything caused by my PR .. but most of the issues camed from upstream: b600c58

@anki-code
Copy link
Member

Stop

@jnoortheen
Copy link
Member

@xmnlab good then. Thanks for the PR and welcome to the Xonsh community as a contributor.

@jnoortheen jnoortheen merged commit 0c713a0 into xonsh:main Feb 27, 2023
@jnoortheen
Copy link
Member

@anki-code what has happened?

@anki-code
Copy link
Member

anki-code commented Feb 27, 2023

I've tested the changes and I see we lost the reference to the line that causes error.

Now:

pip install git+https://github.com/xmnlab/xonsh@hide-traceback-for-RAISE_SUBPROC_ERROR
xonsh

echo @("""
$XONSH_SHOW_TRACEBACK = False
$RAISE_SUBPROC_ERROR = True

print('LINE 1')
cp nonexisting_file.txt other_name.txt
print("LINE 2")
""") > /tmp/1.xsh

xonsh /tmp/1.xsh
# LINE 1
# cp: cannot stat 'nonexisting_file.txt': No such file or directory

Expected output:

# LINE 1
# cp: cannot stat 'nonexisting_file.txt': No such file or directory
# File "/tmp/1.xsh", line 6, in <module>    <<<<<<<<<<<<<<<<<<<<<
#   cp nonexisting_file.txt other_name.txt  <<<<<<<<<<<<<<<<<<<<<

Without this the debugging is very uncomfortable.

Can we add the line number with the error?

@jnoortheen
Copy link
Member

Without this the debugging is very uncomfortable.
Can we add the line number with the error?

Yes it should show that info. I think the first line of the traceback has that info. In the if-clause we can add else branch to show the first line of the traceback

@anki-code
Copy link
Member

Please add.

@xmnlab xmnlab deleted the hide-traceback-for-RAISE_SUBPROC_ERROR branch February 27, 2023 19:59
@xmnlab
Copy link
Contributor Author

xmnlab commented Feb 27, 2023

ok I can open a follow up PR for that! thanks!

@jnoortheen
Copy link
Member

My current suggestion is

if XSH.env.get("XONSH_SHOW_TRACEBACK"):
  traceback.print_exception(*exc_info)
else:
  traceback.print_exception(*exc_info, limit=1)

but this will print more lines. It would need to be formatted.

chmate pushed a commit to chmate/xonsh that referenced this pull request Oct 9, 2023
… and $RAISE_SUBPROC_ERROR=True (xonsh#5066)

* fix: Add and extra condition for print traceback from subprocess

* add news

* Apply pre-commit hooks

* Fix initial implementation using sys.exit

* Update pr-5066.rst

* Remove sys.exit

---------

Co-authored-by: Andy Kipp <anki-code@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.

None yet

4 participants