[execute_process] emulate_tty configurable and defaults to true

[stonier]

Attempting resolution of #188. Can test with the example included.

@wjwwood is this in line with what you were expecting?

[wjwwood]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

@stonier thanks for updating the pr, but it looks like flake8 is complaining still in our CI. :(

[stonier]

Your CI looks to be somewhat ahead of my bionic installation which passes the flake 8 test (is it due solely to pytest versions? I have 3.3.2, CI has 4.6.3). I've taken a stab at guessing what the fix should be - can you trigger the CI run?

[clalancette]

I've just rebased this branch, and added some fixes for the flake8 errors; it seems to pass for me locally now. Here's new CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

Thanks @clalancette and @stonier, yeah I think our CI uses flake8 from pypi (latest).

[wjwwood]

It does appear, however, that a related test is failing on Windows.

[stonier]

Just for reference (because that log is a PITA to read):

UnicodeEncodeError: 'charmap' codec can't encode character '\\u2713' in position 154: character maps to <undefined>
....
File "C:\\J\\workspace\\ci_windows\\ws\\src\\ros2\\launch\\launch\\launch\\launch_service.py", line 184, in __process_event
    "processing event: '{}' \u2713 '{}'".format(event, event_handler))

launch/launch/launch/launch_service.py

Line 184 in 81476cb

"processing event: '{}' ✓ '{}'".format(event, event_handler))

[wjwwood]

Weird, that's a debug message. Not sure why that would try to print only after these changes... I don't see a failure on master. I wonder if this is a red herring and there's another issue? I'll look into it quickly.

[stonier]

Implemented option 2 as discussed in the diff thread (abort emulating tty when passed emulate_tty=True on windows, adjust test expectations for windows with comments, raise an issue).

This closes #188, triggers new issue #268.

[clalancette]

Another CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[stonier]

Pushed a commit for the linting. Worth noting though, it's something of a challenge getting the linting to work in the same way as it does on CI - I haven't been able to do so yet. Without going into the CI console log to try and reproduce the environment setup step-by-step are there more convenient instructions somewhere?

It's worth noting that on a dashing-bionic install:

• System dependencies defined by ament_cmake_flake8 do not create the same environment as the CI tests (e.g. does not even bring in python3-flake8-docstrings)
• If someone does happen to have python3-flake8-docstrings installed on their system, the tests fail in a multitude of ways (different version, different configuration?)

Does it really need to lean on non-system versions?

[wjwwood]

are there more convenient instructions somewhere?

This part of the setup instructions have you install flake8-docstrings and some other things, from pip:

https://index.ros.org/doc/ros2/Installation/Dashing/Linux-Development-Setup/#install-development-tools-and-ros-tools

Which is what our CI does.

Our binary instructions exclude things needed to run tests, because the assumption is that if you're installing the binaries that you are building on top of it and will install your own test dependencies if you want to run linters on your packages. We also build from source when drafting changes to the core packages, since that is more likely to be up-to-date than the binary packages and because that's what we need to do in order to emulate CI's checks.

You could also use our docker images if you want a known good environment in which to test.

Sorry you had issues getting the linters to behave, that can be frustrating.

System dependencies defined by ament_cmake_flake8 do not create the same environment as the CI tests (e.g. does not even bring in python3-flake8-docstrings)

That's because we install it from pip and debian packages cannot install dependencies from pip.

If someone does happen to have python3-flake8-docstrings installed on their system, the tests fail in a multitude of ways (different version, different configuration?)

That's correct, it's an out of date version with bugs/false positives in it.

Does it really need to lean on non-system versions?

Unfortunately, yes I believe it does. You could argue that we don't need all of these linter extensions in the first place, which I would tend to agree with a bit (thought obviously others on the team thought they were worthwhile), but if you really want to push against this, I'd try to find the point where these linters were added and comment there.

[wjwwood]

One question, otherwise lgtm.

[stonier]

Does it really need to lean on non-system versions?

Unfortunately, yes I believe it does. You could argue that we don't need all of these linter extensions in the first place, which I would tend to agree with a bit (thought obviously others on the team thought they were worthwhile), but if you really want to push against this, I'd try to find the point where these linters were added and comment there.

It's not a big drama, but it does certainly complicate things and the ROS infrastructure promises that one has come to rely on start to fall away.

The implication here is that I can install ament debian packages for linting, but they have only been verified to work with hidden pip dependencies. There is no guarantee that they will even be functional out of the box without their pip dependencies.

Additionally, the test_depends elements in package.xml are no longer a true representation and subsequently a rosdep install will no longer get everything you need to build and run your tests.

Thanks for the link back to the installation docs though.

[wjwwood]

There is no guarantee that they will even be functional out of the box without their pip dependencies.

I agree, though I also see the counter point of how constraining it is when we cannot use the tools we need to get the job done (not saying that's the case here, but it has been in similar cases in the past).

Perhaps @dirk-thomas can comment on this, since he spends a lot of time working on these tools and dependencies. (he's on vacation right now though, so it might be later)

Additionally, the test_depends elements in package.xml are no longer a true representation and subsequently a rosdep install will no longer get everything you need to build and run your tests.

rosdep will still work, since we map the rosdep key's in the package.xml to pip not to apt in these cases. The only thing that does not work is that apt packages cannot not automatically install them. This has come up over and over again in ROS 1, e.g.: ros/catkin#746

[wjwwood]

lgtm, thanks!

[wjwwood]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

Edit: needed to recreate the repos file since we pinned the fastrtps version yesterday.

[wjwwood]

I had to rebase this because of how our CI works, but here's a new round:

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

Thanks @stonier for the contribution and for taking time to iterate with us!

[wjwwood]

Unfortunately this broke a bunch of our tests last night, so I'll revert and fix it up.

[dirk-thomas]

Unfortunately this broke a bunch of our tests last night, so I'll revert and fix it up.

Just for the record: the tests failing to match their output seem to be related to this change http://build.ros2.org/view/Eci/job/Eci__nightly-fastrtps_ubuntu_bionic_amd64/21/testReport/