Commands in the release module always return exit code 0, even if they fail

[julian-pani]

Hi,
When executing commands such as nbdev_pypi from a shell (which get routed to nbdev.release:release_pypi), the commands always return exit code 0 regardless of the errors happening inside the command.

I ran into this issue while using nbdev_pypi inside my CI/CD. If I forget to bump the version, for example, the twine upload command will fail with a "409 Conflict" error, but the nbdev_pypi command still returns error code 0 (success).
This is problematic, because there is no way to catch this error and do something useful (like fail the CI/CD build).

The code for release_pypi uses os.system calls like:

system(f'twine upload --repository {repository} {_dir}/dist/*')

https://github.com/fastai/nbdev/blob/4af4d479c78880f4a18af4254b119f8af8b3a8a4/nbdev/release.py#L307-L313C68

As far as I can see, the call to os.system() returns the exit code as a 0/1 but does not throw an exception.

I'm happy to contribute a pull request to fix this, but would like some guidance on what is the right way to go about it.
Options I see:

1. Change code to use subprocess instead of os.system()
2. Keep using os.system, but instead of calling it directly - add a wrapping function like execute_throw_on_error() which will check the response code and throw exception if it's not 0.
3. Somehow handle it in a more generic way in the the call_parse decorator (but what do we do if there are multiple calls to os.system and we want to fail if either fails?)

Thanks!

Julian.

[deven367]

I think the way to deal with this issue would be to run these either through subprocess.run and catch the stderr or subprocess.Popen. If we decide to go this route, we would need to update all the instances in the library (I checked, there are a total of 9 instances). Any thoughts @hamelsmu, @jph00?

[deven367]

@hugetim Can you take a look at this?

[hugetim]

I'm flattered to be asked but don't have anything to add on this. My knowledge of the nbdev codebase is limited pretty narrowly to the piece your recent PR touched, and I have only a shallow understanding of this aspect of that function. Well, hmm, is this helper function there in line with the approach you are advocating?

nbdev/nbdev/quarto.py

Lines 25 to 27 in 6c7d240

	def _sprun(cmd):
	try: subprocess.check_output(cmd, shell=True)
	except subprocess.CalledProcessError as cpe: sys.exit(cpe.returncode)

[deven367]

Yes, it's a similar approach, I'm mostly planning to use a different method either subprocess.run or subprocess.Popen