unix/main: Use standard pyexec/repl for unix port. #12802
Conversation
|
Thanks for this, it's a good clean up. I actually attempted it before, see #6008. The approach here is much simpler, so maybe it's a good first step to make. |
|
Ah interesting, I did see some of the return handling and kind of assumed it shouldn't matter - if someone is dealing with the repl (live coding) they're unlikely to care about the exit code? I didn't consider the SystemExit interrupt at all, haven't looked at how that behaves but probably should! If running a python file directly then certainly the exit code is important but that shouldn't be impacted by this. |
e4cc4f5 to
2c1c7d8
Compare
|
Ah, I might need to read through the other PR discussion a bit more... but in the mean time this is a little off: Edit: |
2c1c7d8 to
55032e9
Compare
|
See also #12807 |
12f5770 to
7a0825f
Compare
|
I ran into a tricky issue when trying to fix the unit tests; the banner on Unix port has changed a little with this MR (now consistent with other ports) so unit test exp needed to be updated to match. I initially tried just copying the results/*.out files to replace the original .exp ones, but the tests still failed! There were inconsistent newlines in the files that tripped up the test.
|
ports/windows/micropython.vcxproj
Outdated
| @@ -88,6 +88,7 @@ | |||
| <ClCompile Include="@(PyCoreSource)" /> | |||
| <ClCompile Include="@(PyExtModSource)" /> | |||
| <ClCompile Include="$(PyBaseDir)shared\readline\*.c" /> | |||
| <ClCompile Include="$(PyBaseDir)shared\runtime\pyexec.c" /> | |||
There was a problem hiding this comment.
Super minor but can you move this one line down so it's aplhabetically?
There was a problem hiding this comment.
Ah whoops, I honestly thought I was alreadyt putting it in order, eyes blurred readline/runtime and completely missed runtime\gchelper_generic below!
tests/run-tests.py
Outdated
| @@ -232,6 +233,11 @@ def send_get(what): | |||
| with open(test_file, "rb") as f: | |||
| # instead of: output_mupy = subprocess.check_output(args, stdin=f) | |||
| master, slave = pty.openpty() | |||
| # disable auto newline conversion on the pty terminal. | |||
| attrs = termios.tcgetattr(slave) | |||
There was a problem hiding this comment.
You're probably aware but this doesn't exist on windows. Isn't there a way to just do a replace("\r\n", "\n") before comparing results, or something like that?
There was a problem hiding this comment.
Oh, I thought pty itself wasn't avaialble either so it would be a moot point?
I initially thought of just adding another newline replace, but thought fixing the problem at the source was cleaner than further fiddling with test results.
There was a problem hiding this comment.
Yeah it looks like these repl_* tests are already skipped on windows master builds https://ci.appveyor.com/project/dpgeorge/micropython/builds/48536058/job/4o16bx3lnvsu7nbn
There was a problem hiding this comment.
Oh, I thought pty itself wasn't avaialble either so it would be a moot point?
Yes, but
I initially thought of just adding another newline replace, but thought fixing the problem at the source was cleaner than further fiddling with test results.
Cleaner perhaps, but: reaonsing that suppose we do get cmdline tests working for the windows port, like with some other code, then that would also need a similar fix. Whereas the more general case of the tests being written like 'whatever you throw at me I'm going to make newlines consistent first because I know that can be an issue' seems the more robust way plus keeps everything nicely in one place?
7a0825f to
ad27be1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12802 +/- ##
=======================================
Coverage 98.54% 98.54%
=======================================
Files 169 169
Lines 21898 21898
=======================================
+ Hits 21579 21580 +1
+ Misses 319 318 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ad27be1 to
02fb334
Compare
d08fe6e to
554ba06
Compare
|
Regarding the comment above about We need to retain the existing behaviour, so we don't break existing usage, and because that's how CPython works. Then it's a separate question how to handle this on bare-metal targets. But for now we shouldn't change the unix behaviour on this. |
|
Code size report: |
Thanks, yes I hadn't realised this behavior had been changed inadvertantly. I've now fixed it in the latest push. ... which matches cpython correctly:
Well it looks to me like this should be treated as a soft reset. micropython/extmod/modmachine.c Line 66 in 358e501 The current push also works this way: |
Right, it would be a nice clean up to get rid of that variable, and also change the behaviour of Because this is a change in user-facing behaviour, I've split that change out in #15486. |
03e992d to
95bce29
Compare
|
This has been rebased to master, taking into account the SystemExit stuff already merged. |
|
Testing this on the unix coverage variant, passing I suggest just copying the following bit into the relevant place in #if defined(MICROPY_UNIX_COVERAGE)
// allow to print the parse tree in the coverage build
if (mp_verbose_flag >= 3) {
printf("----------------\n");
mp_parse_node_print(&mp_plat_print, parse_tree.root, 0);
printf("----------------\n");
}
#endif |
95bce29 to
8b8b730
Compare
Thanks, I wasn't aware of this development feature. I also hadn't noticed the amount of overlap between pyexec/parse_compile_execute function and unix/main/execute_from_lexer() but there's probably not much to be really gained by consolidating the two. I've made the change now and assume it's working: |
|
It looks like there are still a couple of things missing in pyexec.c that the unix port has:
|
This is why #6008 is quite a bit bigger than this PR, and why it was still in progress, because of all these details 😄 |
8b8b730 to
5d39ed7
Compare
|
@andrewleech there are still two of my outstanding points above to address, and also @stinos comment about newlines. Did you want to address those? Alternatively I can merge #16141 now to get raw REPL support on the unix port, then this PR can go in after that, when it's ready. |
|
I would very much like to get this PR finished to unblock aiorepl use, I'll reprioritize it in my backlog cleanups effort! Previously I didn't like the idea of a blanket replace it newlines like @stinos suggested however after battling inconsistent newline matching in some cmdline tests just the other day I'm now far more interested in the simple/broad fix! |
|
I haven't reviewed the new commits yet (I will though) but pushed them in car anyone else was interested: I literally just asked Claude code to: code review this branch, then use the gh api to read all review comments then write a tech plan for how to implement this, think deep and find needed references. Then implement the plan, committing each feature to a new commit. These new commits is what it came up (including writing and running a few rounds of tests) |
0f97a2f to
82a1c84
Compare
This improves repl usage consistency across ports. Only enabled when MICROPY_USE_READLINE == 1 (default). Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
When MICROPY_PY___FILE__ is enabled and parsing file input, set the global __file__ variable to the source filename. This matches the behavior of the unix port and provides the current filename to the executing script. Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
|
@dpgeorge this should be ready for re-review. This PR does not include the unhandled exception hooks that you had in #6008 but I feel that should be a separate feature / pr anyway if it's still needed by others? There's a minor compatibility update to aiorepl in micropython/micropython-lib#1016 which has been tested with this PR and seems to behave as expected. Without that change aiorepl needs crtl-enter to execute / send a line. |
This adds EXEC_FLAG_COMPILE_ONLY to parse_compile_execute() which compiles the code but doesn't execute it. A new API function pyexec_file_with_flags() is also added to allow passing execution flags from ports. This is needed for unix port's -X compile-only option. Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
Update do_file() to use pyexec_file_with_flags() and pass the compile-only flag when -X compile-only is specified. This completes the integration of the new pyexec functionality with the unix port's command-line options. Also handle return code conversion between pyexec and unix port conventions. Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
Add a general normalize_newlines() function that handles newline variations (\\r\\r\\n, \\r\\n) to \\n while preserving literal \\r characters that are part of test content. This provides a robust solution for cross-platform test compatibility, particularly addressing PTY double-newline issues that can occur with some terminal implementations. The function is applied to all test output before comparison, eliminating platform-specific newline issues. Includes a unit test to verify the normalization behavior. Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
This improves repl usage consistency across ports.
Only enabled when MICROPY_USE_READLINE == 1 (default).
In particular I noticed that
aiorepldidn't work properly on the unix port currently, it would echo the "just entered line" on a newline each time you hit enter, eg.I feel there were also a few other inconsistencies with how I'm used to the repl working on the stm port / via mpremote which are alsos hopefully resolved now.
I've got an optional couple of lines to restore/reset stdio_raw mode when running code - however if raw mode is disabled the aiorepl issue above remains. I think for consistency with other ports it's better to keep stdio in raw mode, but not sure if this is likely to have other side effects?
This PR consilidates the repl functionality and fixes the above aiorepl glitch.