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

DVC exits with code 0 even if there was an error #2850

Closed
peper0 opened this issue Nov 26, 2019 · 8 comments · Fixed by #2856
Closed

DVC exits with code 0 even if there was an error #2850

peper0 opened this issue Nov 26, 2019 · 8 comments · Fixed by #2856
Assignees
Labels
bug Did we break something? p0-critical Critical issue. Needs to be fixed ASAP.

Comments

@peper0
Copy link
Contributor

peper0 commented Nov 26, 2019

Tested with dvc 0.70 from apt on Ubuntu:

$ lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 18.04.2 LTS
Release:	18.04
Codename:	bionic

Can be easily reproduced with docker. The following example shows pulling from a nonexistent repository but the same problem is with errors of any kind.

Steps to reproduce
Create a following Dockerfile:

FROM ubuntu:18.04

RUN apt update
RUN apt install -y wget
RUN wget https://dvc.org/deb/dvc.list -O /etc/apt/sources.list.d/dvc.list
RUN apt update 
RUN apt install -y dvc
WORKDIR /work
RUN dvc init --no-scm
CMD bash -c 'dvc pull -r bleble && echo "command succesfull, but should not be"'

Build it:
$ docker build . -t dvc-bug

Run it:
$ docker run -it dvc-bug

Obtained result

ERROR: failed to pull data from the cloud - config file error: unable to find remote section 'bleble'

Having any troubles?. Hit us up at https://dvc.org/support, we are always happy to help!
command succesfull, but should not be

Expected result
No command succesfull, but should not be text in the output.

Mentioned earlier in #2038

@triage-new-issues triage-new-issues bot added the triage Needs to be triaged label Nov 26, 2019
@efiop efiop added the bug Did we break something? label Nov 26, 2019
@triage-new-issues triage-new-issues bot removed the triage Needs to be triaged label Nov 26, 2019
@efiop
Copy link
Contributor

efiop commented Nov 26, 2019

Hi @peper0 !

Thank you for a very convenient reproducer! Indeed, I can reproduce this bug with binary dvc, but not with dvc installed from pip/brew/conda/etc. As a workaround, could you try installing dvc from one of those sources (i suppose pip will be the most convenient for you) and let us know if that works?

Related #2760 #2768

@efiop
Copy link
Contributor

efiop commented Nov 26, 2019

Looks like even things like dvc non-existing-command don't result in non-zero code for pyinstaller-built images. Investigating...

@efiop efiop added the p0-critical Critical issue. Needs to be fixed ASAP. label Nov 26, 2019
@peper0
Copy link
Contributor Author

peper0 commented Nov 26, 2019

@efiop Can be reproduced with pip installed version when dvc is used as a python module:

python -m dvc pull -r bleble && echo "zero exit code"

However the wrapper script installed in .../bin/dvc fixes this issue. It is probably generated with a fastentrypoints module and handles the return value of main function correctly:

if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
    sys.exit(main())

However, pyinstaller use dvc/__main__.py directly, which contain invalid code (imo):

if __name__ == "__main__":
    main(sys.argv[1:])

peper0 added a commit to peper0/dvc that referenced this issue Nov 26, 2019
@peper0
Copy link
Contributor Author

peper0 commented Nov 26, 2019

I'he just created a merge request of a trivial patch that fixes it for the direct python usage (python -m dvc ...).

I couldn't test it for apt version, since I've had problems with building it.

@peper0 peper0 mentioned this issue Nov 26, 2019
3 tasks
@efiop
Copy link
Contributor

efiop commented Nov 26, 2019

@peper0 oh wow, amazing investigation! Thank you so much! 🙏 I think you are absolutely right about it. I'll give it a try in a second.

EDIT: tested and it works like a charm! 🎉

efiop added a commit that referenced this issue Nov 26, 2019
@efiop
Copy link
Contributor

efiop commented Nov 26, 2019

@peper0 I'll trigger a new release today, thank you!

@efiop
Copy link
Contributor

efiop commented Nov 27, 2019

@peper0 0.71.1 is out, please upgrade and give it a try 🙂 Thank you so much!

@peper0
Copy link
Contributor Author

peper0 commented Nov 27, 2019

It's good now. Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Did we break something? p0-critical Critical issue. Needs to be fixed ASAP.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants