Page MenuHomePhabricator

[seven] Remove trailing characters from command output.

Authored by JDevlieghere on Jan 25 2019, 5:59 PM.



When running the test suite on macOS with Python 3 we noticed a difference in behavior between Python 2 and Python 3 for seven.get_command_output. The output contained a newline with Python 3, but not for Python 2. This resulted in an invalid SDK path passed to the compiler.

There were only two actual usages left of this module so I propose to remove it and have a simple, local implementation for get_command_output.

Diff Detail


Event Timeline

JDevlieghere created this revision.Jan 25 2019, 5:59 PM

I don't have a strong preference between using a separate module for interopability between python versions and implementing the functions as part of dotest if there are few of them, but I have to agree with @zturner that if we have more than a couple of functions, it makes sense to add them to an explicit module.

Incidentally, I think we do have more than a couple of functions but they are not in the module right now - most likely because people who added them (me included) didn't know any better.


I suspect that if you end up using your implementation, you'll have to add universal_newlines=True, so that it all works correctly across platform (and on Windows)

51 ↗(On Diff #183674)

This version of get_command_output no longer uses six for python 2, does it still work correctly for both python 2 and python 3?

+1 for keeping Anything that moves code out of is a good thing.

  • Remove trailing characters from get_command_status_output.
  • Remove unused seven imports.

(I'll land this as two separate commits)

JDevlieghere retitled this revision from [testsuite] Remove seven dependency to [seven] Remove trailing characters from command output..Jan 28 2019, 9:01 AM
stella.stamenova accepted this revision.Jan 28 2019, 9:49 AM
This revision is now accepted and ready to land.Jan 28 2019, 9:49 AM
zturner accepted this revision.Jan 28 2019, 9:53 AM
This revision was automatically updated to reflect the committed changes.