capture errors from cargo metadata
#254
Conversation
|
Thanks for the PR, please format code with black. |
setuptools_rust/extension.py
Outdated
| env = os.environ.copy() | ||
| try: | ||
| payload = subprocess.check_output( | ||
| metadata_command, env=env, encoding="latin-1" |
There was a problem hiding this comment.
The default is to inherit the current environment? I don't think env=env is necessary?
There was a problem hiding this comment.
Fair point, I just wanted to make sure env definitely is present, but will remove.
setuptools_rust/extension.py
Outdated
| metadata_command, env=env, encoding="latin-1" | ||
| ) | ||
| except subprocess.CalledProcessError as e: | ||
| msg = f"cargo failed with code: {e.returncode}\n{e.output}" |
There was a problem hiding this comment.
Should this also include stderr? Also it would be nice to say cargo metadata instead of just cargo.
There was a problem hiding this comment.
As I wrote, I copied the setup from build.py. Happy to add e.stderr here (and also in build.py if desired)
There was a problem hiding this comment.
Made these adaptations now.
|
Thanks for the quick review! |
h-vetinari
changed the title
cargo metadata and pass in envcargo metadata
setuptools_rust/extension.py
Outdated
| self._cargo_metadata = json.loads(subprocess.check_output(metadata_command)) | ||
|
|
||
| try: | ||
| payload = subprocess.check_output(metadata_command, encoding="latin-1") |
There was a problem hiding this comment.
Ah, I'm really sorry to have missed on the first read, I think we might need to opt-in to capturing stderr:
| payload = subprocess.check_output(metadata_command, encoding="latin-1") | |
| payload = subprocess.check_output(metadata_command, encoding="latin-1", stderr=subprocess.PIPE) |
There was a problem hiding this comment.
(could also use stderr=subprocess.STDOUT to merge the two together so that the error could just use e.stdout, I think both merged and separate have advantages and disadvantages so happy to go with whichever you prefer)
There was a problem hiding this comment.
Good point, thanks!
I preferred the separate versions (no reliance that merge works correctly/legibly), but happy to change if desired.
There was a problem hiding this comment.
Ah, never mind, the merging is apparently the recommended (or at least documented way): https://docs.python.org/3/library/subprocess.html#subprocess.check_output
There was a problem hiding this comment.
As an added benefit, this makes the exception message (& diff) much smaller. :)
There was a problem hiding this comment.
Well, the merge was what's causing the json errors... It adds the logs into the output, making the json unparseable, e.g.:
Error parsing output of cargo metadata as json; received:
b' Updating crates.io index\n Downloading crates ...\n Downloaded unindent v0.1.8\n Downloaded target-lexicon v0.12.3\n Downloaded pyo3-macros v0.16.5\n Downloaded cfg-if v1.0.0\n Downloaded scopeguard v1.1.0\n Downloaded unicode-xid v0.2.2\n Downloaded proc-macro2 v1.0.36\n Downloaded indoc v1.0.4\n Downloaded instant v0.1.12\n Downloaded once_cell v1.10.0\n Downloaded lock_api v0.4.6\n Downloaded parking_lot v0.11.2\n Downloaded bitflags v1.3.2\n Downloaded pyo3-ffi v0.16.5\n Downloaded quote v1.0.16\n Downloaded syn v1.0.89\n Downloaded pyo3-macros-backend v0.16.5\n Downloaded smallvec v1.8.0\n Downloaded redox_syscall v0.2.11\n Downloaded parking_lot_core v0.8.5\n Downloaded pyo3 v0.16.5\n Downloaded winapi v0.3.9\n Downloaded libc v0.2.121\n Downloaded pyo3-build-config v0.16.5\n Downloaded winapi-x86_64-pc-windows-gnu v0.4.0\n Downloaded winapi-i686-pc-windows-gnu v0.4.0\n{"packages":[{"name":"bitflags",
There was a problem hiding this comment.
Ah, I see!
Now that I realise that these updating / downloading messages come through stderr - it's probably quite nice to get them as output by default (i.e. not capture stderr)?
Could maybe check the --quiet flag - if set, capture and include stderr in the exception message, otherwise just don't capture stderr? What do you think of that?
I think we're pretty much there now - thanks for the patience / iteration on this!
There was a problem hiding this comment.
I went back to trying the non-merged version for now. :)
There was a problem hiding this comment.
This is looking better now. :)
|
I struggle to understand how any of the recent commits could make the json parsing fail (and the CI passed for b6752c8...) |
|
Looks fine to me, thanks very much! |
Should fix #253.
The setup is copied largely from what's already in
build.py, but usingDistutilsSetupErrorinstead ofCompileError(in line with the rest of the error reporting inextension.py). Also, I don't think duplicating the_prepare_build_environmentis necessary just forcargo metadata...?