erlang
Add the ERL_SYS_SHELL env variable to point to system shell for os:cmd()
CT Test Results
2 files 70 suites 1h 5m 58s :stopwatch: 1 554 tests 1 312 :white_check_mark: 242 :zzz: 0 :x: 1 771 runs 1 496 :white_check_mark: 275 :zzz: 0 :x:
Results for commit bb270850.
:recycle: This comment has been updated with latest results.
To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.
See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.
Artifacts
// Erlang/OTP Github Action Bot
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hello! Thanks for the PR!
The variable should be a kernel configuration parameter that is set to the correct value when kernel is started. It also needs to be documented here and a testcase needs to be added to os_SUITE.
Thanks for the update! I've pushed a commit that renames the variable to os_cmd_shell which I think better describes what it does and also moved the shell detection logic to happen when we start instead of everytime we do os:cmd.
Would you mind updating the docs of os:cmd/2 to point to the docs for the configuration parameter so that users know where to find it?
Feel free to squash my commit into yours if you agree with my changes.
I'm a bit confused by the test results. As I see one subtest in shell_history_custom_error/1 test failed with the timeout, but a similar subtest passed just fine. Moreover I don't see how this part of the functionality can be affected by my changes, and locally the test passed just fine. Could you give me a hint on what might cause the test to fail?
I added os:internal_init_os_cmd/1 because locally the tests that contain os:cmd/2 (at least some of them) crashed with badmatch at {ok, Shell} = application:get_env() in os:mk_cmd/2.
I will add the test with ?CT_PEER
I added os:internal_init_os_cmd/1 because locally the tests that contain os:cmd/2 (at least some of them) crashed with badmatch at
{ok, Shell} = application:get_env() in os:mk_cmd/2.
How odd, can you remove them and see if that happens in github ci as well?
Now when I run local tests on the latest commit I again get {badmatch, undefined} for os:mk_cmd(), but not in all testsuites that use os:cmd/2. More so, I get errors in tests that passed here on 5280771 commit, and were not touched since. Looks weird...
Yes, I can see the problem in github actions tests as well. I would guess that some testcase in application_SUITE clears the env of kernel for some reason...
I found the problem, it was that we did not re-init the variable when kernel:config_change/3 was called.
I pushed a fix for that. I also scratched an itch that was semi-related to this and that was that I don't think we should allow this value to be changeable at run-time. So I moved the value to persistent term so that updating the application parameter in run-time has no effect.
Now as I see all the tests are passing. Is anything expected from my side? One of our customers is eager for getting the patch in their upcoming release, so if the code change is OK I will apply it to our OTP version while waiting for the formal release.
I've put it into testing so that we get some more platform coverage (windows, *bsd, solaris etc). If all tests pass I will merge this PR for release in 27.2. So nothing is expected from you unless something fails somewhere where I think you can help out.