search

aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
437 stars 192 forks source link

`exec_command_wait` should return stderr and stdout as bytes rather than strings? #3814

Closed giovannipizzi closed 6 months ago

giovannipizzi commented 4 years ago

E.g., if the command you run is cat SOMEFILE.pdf, these line will most likely crash: https://github.com/aiidateam/aiida-core/blob/a34b3566b5e98ebedb58f0f142cbf54808d146c0/aiida/transports/plugins/ssh.py#L1262-L1263 (same for the local transport).

Do you agree?

However, changing the API is backward incompatible... Maybe we should just deprecate exec_command_wait, have a exec_command_wait_bytes, and have the whole code use it instead? Or we just fix this in 2.0? Or other suggestions?

Mentioning @sphuber @dev-zero @greschd @ConradJohnston @ramirezfranciscof for info and/or comments

sphuber commented 4 years ago

I agree that we should not guess the encoding but simply return the bytes and leave it up to the caller. As for the integration pathway: I think adding a deprecation warning to say behavior will change in 2.0.0 and change it then, is fine, as long as the delay is acceptable. Since we have been doing this for a long time with so far no issues, I think it is fine to delay the fix

giovannipizzi commented 4 years ago

After discussion in person, here is the proposed solution: already in #3787, define a new function returning bytes (e.g. exec_command_wait_bytes). Internally use always this, and have the exec_command_wait return strings as it currently does for backward compatibility. Put a deprecation warning stating that in 2.0 the function will start returning bytes (and the internal _bytes version will be dropped).

@sphuber Thinking to this, I realise that on one side we might want to make the internal function _exec_command_wait_bytes hidden - if we don't, this becomes public API, but then we should put a deprecation warning saying that it will be dropped in 2.0, and then both functions would return a deprecation warning... However, this is not good as we then would be calling internal _ methods from AiiDA. Or we keep them both in 2.0 (exec_command_wait and exec_command_wait_bytes, doing the same)? Or any better suggestion? (The easiest would probably be to decide a new name different from exec_command_wait that will return bytes, we implement now and will remain in 2.0... Maybe simply exec_wait or exec_and_wait?

sphuber commented 6 months ago

This was fixed in #3787