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

Adjust mix nerves.system.shell for OTP 26 #897

Merged
merged 1 commit into from Sep 12, 2023
Merged

Conversation

jjcarstens
Copy link
Member

@jjcarstens jjcarstens commented Jul 31, 2023

OTP 26 made some big changes around serial interface and it breaks the mix nerves.system.shell task. In the end, the main issue is that we no longer have an easy way to take over the STDIN of the task to forward it to a shell running in the system build directory.

Many attempts were made to recapture STDIN, but decided that for now we need to move on. Instead, this adjusts the task to do all the setup for the build directory as before and outputs a warning to the user for the command that can be manually run in order to get the same shell that was previously started with mix nerves.system.shell

Docker error
image

Local (Linux) error
image

@fhunleth
Copy link
Member

What do you do to continue the build after you run Docker?

@jjcarstens
Copy link
Member Author

The output docker command is the last step of "starting the system shell" - You can do the typical things there such as make menuconfig or just use make to compile. Everything is referenced in a build volume.

Later one, if one uses the system as a path dependency then those same build artifacts are used with mix compile and mix firmware

@jjcarstens
Copy link
Member Author

@fhunleth This is ready for another review. I adjusted it to output the usable command at the end. A really hacky workaround is a user could do eval "$(mix nerves.system.shell | tail -n 1)" to get the same effect in OTP 26, or just copy/paste the output command in the error.

@jjcarstens jjcarstens force-pushed the 893-fix-shell-task branch 2 times, most recently from f814107 to 227654e Compare September 5, 2023 13:35
@guillego
Copy link

guillego commented Sep 5, 2023

This is a great improvement! Not the smooth experience of OTP 25 but much better than the cryptic error message.

The only caveat I have is this message after running the long docker command:

$ docker run --rm -it -w /nerves/build --env UID=501 --env GID=20 --mount type=volume,src=nerves_system_rpi0_video-iJFrgQHhlZ5Yv6f8,target=/nerves/build --mount type=bind,src=/Users/guille/.nerves/dl,target=/nerves/dl --mount type=bind,src=/Users/guille/personal/nerves_system_rpi0_video,target=/nerves/env/nerves_system_rpi0_video --mount type=bind,src=/Users/guille/personal/nerves_system_rpi0_video/deps/nerves_system_br,target=/nerves/env/platform --env NERVES_BR_DL_DIR=/nerves/dl -v /private/tmp/com.apple.launchd.R14n1eLnGE/Listeners:/ssh-agent -e SSH_AUTH_SOCK=/ssh-agent ghcr.io/nerves-project/nerves_system_br:1.23.3 /bin/bash -c "/nerves/env/platform/create-build.sh /nerves/env/nerves_system_rpi0_video/nerves_defconfig /nerves/build ; exec /bin/bash"
Setting User
UID: 501
GID: 20
Switching user
make: Entering directory '/nerves/env/platform/buildroot-2023.02.3'
  GEN     /nerves/build/Makefile

WARNING: unmet direct dependencies detected for BR2_PACKAGE_ERLANG
  Depends on [n]: BR2_USE_MMU [=y] && !BR2_STATIC_LIBS [=n] && BR2_TOOLCHAIN_HAS_THREADS [=y] && BR2_PACKAGE_ERLANG_ARCH_SUPPORTS [=n]
  Selected by [y]:
  - BR2_PACKAGE_NERVES_CONFIG [=y]

WARNING: unmet direct dependencies detected for BR2_PACKAGE_ERLANG
  Depends on [n]: BR2_USE_MMU [=y] && !BR2_STATIC_LIBS [=n] && BR2_TOOLCHAIN_HAS_THREADS [=y] && BR2_PACKAGE_ERLANG_ARCH_SUPPORTS [=n]
  Selected by [y]:
  - BR2_PACKAGE_NERVES_CONFIG [=y]
#
# configuration written to /nerves/build/.config
#
make: Leaving directory '/nerves/env/platform/buildroot-2023.02.3'
------------

Build directory successfully created.

Configuration: /nerves/env/nerves_system_rpi0_video/nerves_defconfig

Next, do the following:
   1. cd /nerves/build
   2. make

For additional options, run 'make help' in the build directory.

IMPORTANT: If you update nerves_system_br, you should rerun this script.
nerves@b8054632fe06:/nerves/build$

I think this last part could be a bit more clear:

Configuration: /nerves/env/nerves_system_rpi0_video/nerves_defconfig

Next, do the following:
   1. cd /nerves/build
   2. make

For additional options, run 'make help' in the build directory.

IMPORTANT: If you update nerves_system_br, you should rerun this script.

The command already leaves you on /nerves/build so I'm not sure the cd command is needed.
Also I ran make and since the prompt looks different from OTP25 I wasn't sure if I could run make menuconfig like before (I tried and it's in fact the right place to run it).
Perhaps it could be nice to add a message indicating that here is where we run make menuconfig, etc?

@guillego
Copy link

guillego commented Sep 5, 2023

Also, not sure how much of a problem this is, but since the shell used to run within iex itself, exit it was "as intuitive" as pressing ctrl+c. I see now that we are in a bash prompt inside a docker so the way to exit is to type exit.
Perhaps there could be a message indicating this? Not sure if it's over doing it, but perhaps someone might appreciate it.

@jjcarstens
Copy link
Member Author

@guillego That output is all part of the create-build.sh script that gets run. Previously, it was directing to > /dev/null to hide all this, but I think part of the output should be considered, like the unmet dependencies

make: Entering directory '/nerves/env/platform/buildroot-2023.02.3'
  GEN     /nerves/build/Makefile

WARNING: unmet direct dependencies detected for BR2_PACKAGE_ERLANG
  Depends on [n]: BR2_USE_MMU [=y] && !BR2_STATIC_LIBS [=n] && BR2_TOOLCHAIN_HAS_THREADS [=y] && BR2_PACKAGE_ERLANG_ARCH_SUPPORTS [=n]
  Selected by [y]:
  - BR2_PACKAGE_NERVES_CONFIG [=y]

WARNING: unmet direct dependencies detected for BR2_PACKAGE_ERLANG
  Depends on [n]: BR2_USE_MMU [=y] && !BR2_STATIC_LIBS [=n] && BR2_TOOLCHAIN_HAS_THREADS [=y] && BR2_PACKAGE_ERLANG_ARCH_SUPPORTS [=n]
  Selected by [y]:
  - BR2_PACKAGE_NERVES_CONFIG [=y]

So the tradeoff is the erroneous cd ... I could be overruled, but I think someone attempting to interact with a system at this level is either A) Used to this or B) Should get used to this and be aware of any warnings during build.

Also, the prompt was previously being rewritten (for better or worse), but the shell is the same. The initial error could maybe be adjusted to make that more clear?

@jjcarstens
Copy link
Member Author

Ha, also funny you bring up ctrl+c - I actually always hated that. The typical idiom with Docker container I thought was to ctrl+d or exit so that ctrl-c can be used as expected in a terminal session

@guillego
Copy link

guillego commented Sep 5, 2023

No issue with the warnings! And I get the ctrl+c thing, but since it's the default way to exit iex, perhaps it would be good to add some comment.
In general I was referring to providing some extra context to the user who sees this for the first time since the experience is quite different from the one in OTP25, something like this:

warnings, info, etc
----

You are now in the nerves shell which is running inside Docker, to exit, type "exit" or press ctrl+d at any point.

Build directory successfully created.

Configuration: /nerves/env/nerves_system_rpi0_video/nerves_defconfig

Next, do the following: 
   1. cd /nerves/build (should be the current path already)
   2. make

For additional options, run 'make help' in the build directory.

Inside this shell is where you can run `make menuconfig` and related commands to configure your buildroot. Please refer to the docs (https://hexdocs.pm/nerves/customizing-systems.html#buildroot-package-configuration) for more information.

IMPORTANT: If you update nerves_system_br, you should rerun this script.

Alternatively we could just add more info in the actual docs to inform of these changes, let me know if you want a hand with those!

@fhunleth
Copy link
Member

fhunleth commented Sep 5, 2023

Regarding:

WARNING: unmet direct dependencies detected for BR2_PACKAGE_ERLANG
  Depends on [n]: BR2_USE_MMU [=y] && !BR2_STATIC_LIBS [=n] && BR2_TOOLCHAIN_HAS_THREADS [=y] && BR2_PACKAGE_ERLANG_ARCH_SUPPORTS [=n]
  Selected by [y]:
  - BR2_PACKAGE_NERVES_CONFIG [=y]

You can ignore that. It's wrong and luckily doesn't actually do anything. It's due to building in an aarch64 container and that's perfectly fine. It's just unexpected from Buildroot's point of view when compiling Erlang. I should be able to push a fix upstream if it hasn't already been addressed.

@jjcarstens
Copy link
Member Author

K, I've adjusted and added the prompts for the docker container. Turns out those warnings are to stderr, so we can leave >/dev/null and everyone wins!

image

I am hesitant to adjust the prompt for local linux builds. Before, it adjust for a transient shell. With this change, it would rewrite the whole active terminal (if fact, it may be desirable to have their configured one now show?)

OTP 26 made some big changes around serial interface and it breaks the
`mix nerves.system.shell` task. In the end, the main issue is that we
no longer have an easy way to take over the STDIN of the task to forward
it to a shell running in the system build directory.

Many attempts were made to recapture STDIN, but decided that for now we
need to move on. Instead, this adjusts the task to do _all_ the setup
for the build directory as before and outputs a warning to the user for
the command that can be manually run in order to get the same shell that
was previously started with `mix nerves.system.shell`
@guillego
Copy link

guillego commented Sep 5, 2023

Looks perfect now! Basically the only difference is that you need to copy and paste the docker command (on Mac), so I think this is quite a good solution! Thanks a lot for working on this @jjcarstens

@jjcarstens jjcarstens merged commit f9789f2 into main Sep 12, 2023
5 checks passed
@jjcarstens jjcarstens deleted the 893-fix-shell-task branch September 12, 2023 01:11
@garazdawi
Copy link

Hi! Just saw your post over at Elixir Forums.

If you could open an issue on https://github.com/erlang/otp describing what you want to achieve, then we would be more than happy to take a look and see if we can help you. Either by figuring out the proper way to configure the new stdin/stdout system, or by adding something to Erlang that allows you to do what you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants