Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pyterm: add native support #20172

Merged
merged 10 commits into from
Dec 22, 2023
Merged

Conversation

OlegHahm
Copy link
Member

Contribution description

I always found it annoying that native is lacking the convenience features of pyterm (e.g., history, concatenating of commands, logging etc). Of course, one could pipe stdin/stdout into something like socat and use pyterm's feature to connect to a TCP server (e.g., socat EXEC:${NATIVEBIN},end-close,stderr,pty TCP-LISTEN:4711,reuseaddr,fork & pyterm -ts 4711) but that's somewhat tedious and not well-aligned with the typical usage.

Hence, this PR will modify the term make target for native so that pyterm is called instead of calling the native binary directly. In turn, pyterm now has a new option which will spawn RIOT native processes as a subprocess (i.e., child process) and communicate to it via pipes.

Testing procedure

Call BOARD=native make term for arbitrary RIOT applications.

Caution: this is not well tested so far - expect the unexpected.

@github-actions github-actions bot added Platform: native Platform: This PR/issue effects the native platform Area: tools Area: Supplementary tools Area: boards Area: Board ports labels Dec 12, 2023
@OlegHahm OlegHahm added Platform: native Platform: This PR/issue effects the native platform and removed Platform: native Platform: This PR/issue effects the native platform Area: boards Area: Board ports labels Dec 12, 2023
@OlegHahm OlegHahm force-pushed the pr/pyterm_native_pipe branch from f9c46db to 801f944 Compare December 12, 2023 20:22
@github-actions github-actions bot added the Area: boards Area: Board ports label Dec 12, 2023
@miri64 miri64 added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Dec 13, 2023
@miri64
Copy link
Member

miri64 commented Dec 13, 2023

Setting the CI: run tests label to make sure they are run on CI execution (I can imagine this PR will make some trouble there...). Other than that it looks fine from my side.

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 13, 2023
@riot-ci
Copy link

riot-ci commented Dec 13, 2023

Murdock results

✔️ PASSED

dd18461 pyterm: native: remove superfluous tap argument

Success Failures Total Runtime
8101 0 8101 10m:09s

Artifacts

@OlegHahm
Copy link
Member Author

There is an issue with riotctrl. I assume it is because the native process now becomes a child process of pyterm when called via make term but I was not quite able to debug this.

@miri64 miri64 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Dec 13, 2023
@benpicco
Copy link
Contributor

benpicco commented Dec 13, 2023

Setting the CI: run tests label to make sure they are run on CI execution (I can imagine this PR will make some trouble there...). Other than that it looks fine from my side.

IIRC that is only for hardware based tests, all native tests should always run as part of CI

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Dec 13, 2023
@benpicco benpicco added CI: run tests If set, CI server will run tests on hardware for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Dec 13, 2023
@OlegHahm OlegHahm force-pushed the pr/pyterm_native_pipe branch from d95fcdf to 13e5387 Compare December 13, 2023 16:40
@github-actions github-actions bot added the Area: doc Area: Documentation label Dec 13, 2023
@OlegHahm
Copy link
Member Author

Okay to squash and remove the WIP label?

@miri64
Copy link
Member

miri64 commented Dec 13, 2023

Yepp, Murdock will take a while anyways, but tools-test now passes.

@miri64 miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Dec 13, 2023
@OlegHahm
Copy link
Member Author

I realized that this change breaks when passing arguments to native (like when using USE_ZEP=1). :-(

@miri64
Copy link
Member

miri64 commented Dec 13, 2023

Mh... maybe, instead of passing parameter by parameter just have a “native parameters” parameter, to which you pass all parameters required for native. This way you can keep the old TERMFLAGS and just put them into the new “native parameters” parameter.

@miri64
Copy link
Member

miri64 commented Dec 13, 2023

Of course that makes port somewhat unnecessary... not sure this is easily made optional

@OlegHahm
Copy link
Member Author

Of course that makes port somewhat unnecessary... not sure this is easily made optional

Well, port seems to be a special case anyway.

@OlegHahm OlegHahm force-pushed the pr/pyterm_native_pipe branch from 102ac75 to f2fc233 Compare December 18, 2023 12:46
tests/net/emcute/Makefile Outdated Show resolved Hide resolved
dist/tools/pyterm/pyterm Outdated Show resolved Hide resolved
@OlegHahm OlegHahm force-pushed the pr/pyterm_native_pipe branch from f2fc233 to dd18461 Compare December 18, 2023 19:00
@OlegHahm
Copy link
Member Author

Anything left to be addressed?

@miri64
Copy link
Member

miri64 commented Dec 20, 2023

What about #20172 (comment) and #20172 (comment)?

@OlegHahm
Copy link
Member Author

Seemed to be unnecessary and would probably complicate the configuration.

@miri64
Copy link
Member

miri64 commented Dec 20, 2023

Seemed to be unnecessary and would probably complicate the configuration.

Mh, but the comment says why socat was used: To not have extra characters in the stream. I can imagine that the tests depend on that. If you insist on pyterm at least set the pyterm flags that are used with the cleanterm target.

@miri64 miri64 added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Dec 20, 2023
@miri64
Copy link
Member

miri64 commented Dec 20, 2023

Just for that I set run tests to also test if the behavior non-native is preserved ;-).

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 20, 2023
@OlegHahm
Copy link
Member Author

Mh, but the comment says why socat was used: To not have extra characters in the stream.

I'm under the impression that this comment is obsolete.

I can imagine that the tests depend on that. If you insist on pyterm at least set the pyterm flags that are used with the cleanterm target.

But as far as I understand the tests are executed using the cleanterm target anyway.

The tests run successful on my PC and in the CI.

From the test code I also see no reason why the tests should fail with pyterm.

@miri64
Copy link
Member

miri64 commented Dec 21, 2023

Since Murdock does not seem to have a problem with that either, I am fine with that explanation.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soft ACK from my side. @benpicco, since this touches also ZEP dispatch, can you please have another look too?

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes native so much nicer 😃

I also don't see how the tests would depend on pyterm - they use their own python based testing harness (e.g. RIOTCtrl).

examples/gnrc_border_router on native also still works as expected.

@OlegHahm OlegHahm added this pull request to the merge queue Dec 21, 2023
Merged via the queue into RIOT-OS:master with commit c0e71b0 Dec 22, 2023
32 checks passed
@benpicco
Copy link
Contributor

Uh looks like tests/build_system/test_tools did not like this

@OlegHahm OlegHahm deleted the pr/pyterm_native_pipe branch December 22, 2023 17:45
@benpicco
Copy link
Contributor

I also don't see how the tests would depend on pyterm

Turns out I was wrong - or rather they depend on the absence of pyterm. (As in they don't use pyterm on the target boards either, but now will always use it for native)

Now for some reason CI did not catch this, but an unrelated PR manages to shake them out.

@benpicco
Copy link
Contributor

arg this also broke make debug on native

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: doc Area: Documentation Area: examples Area: Example Applications Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Platform: native Platform: This PR/issue effects the native platform Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants