Remove print_source_bashrc property

[Zopolis4]

The "trick" is to use exec() to replace the ruby process with a new instance of the parent shell on exit, which sources the relevant configuration file(s).

Tested and working on x86_64 with both bash and fish-- I couldn't get zsh working (at all) in the container, so that hasn't been tested.

Also, given that we don't need it anymore, I removed print_source_bashrc and the handling around that, as well as any other leftover calls to puts source ~/.bashrc and the like.

Run the following to get this pull request's changes locally for testing.

CREW_REPO=https://github.com/Zopolis4/chromebrew.git CREW_BRANCH=whisk crew update

[Zopolis4]

Note that this will not run if you just call crew, as docopt handles the exit in that case and none of the crew at_exit code runs.

[Zopolis4]

This will be pretty verbose on the containers because they print This is the .bashrc whenever the .bashrc is sourced, btw.

[Zopolis4]

Ah-- this hangs the unit tests, because we start a new shell.

hm

[Zopolis4]

Fixed-- set CREW_SCRIPTING to 1 if you are running crew commands in a script from now on.

[supechicken]

Sorry for late response😅

I don't think this is a good approach, as it will spawn "nested" shells after each crew install call (the original shell is still running unless with exec crew install), which is a waste of memory.

If I remember correctly, you cannot simply kill the parent shell as it will cause the crosh window close immediately.

FYI chromebrew/crew-profile-base#6 was my previous attempt on auto-sourcing bashrc after crew install

[Zopolis4]

Yeah, the additional shells aren't great, but this is the best solution I could find.

Ideally, we'd just replace the shell process that spawned crew, but we can't do that if it'll kill the crosh window.

In theory, could we spawn one additional shell process, then continuously replace that?

[Zopolis4]

I'm sure there's a way to check if there have been any changes in the environment that need to be sourced (i.e. try starting a new shell and check if its environment setup is the same as the old one), and then we could only start a new shell based off of that?

That wouldn't eliminate the problem of additional shells, but it would drastically reduce it.

[supechicken]

The window will not close if we run exec bash in the shell spawned with crosh> shell, it closes only when we try to kill -9 the shell.

Ideally, we'd just replace the shell process that spawned crew, but we can't do that if it'll kill the crosh window.

Why we need to replace the shell at all :) If we can run exec bash in the parent shell then we should be able to source /usr/local/etc/profile while keeping the current shell (variables will be override anyways).

I'm sure there's a way to check if there have been any changes in the environment that need to be sourced (i.e. try starting a new shell and check if its environment setup is the same as the old one),

We can use something like inotifywait to monitor if there are any changes in /usr/local/etc/env.d or with sha256sum /usr/local/etc/env.d/* and compare the result

[Zopolis4]

The problem is that we can't run exec bash in the parent shell since we're still in crew, as far as I'm aware.

[supechicken]

The problem is that we can't run exec bash in the parent shell since we're still in crew, as far as I'm aware.

Although we cannot call exec bash or source ~/.bashrc directly in crew, we can call the parent shell to do it by using custom signal traps.

We can use SIGUSR1 as the signal for source ~/.bashrc, like this:

# Add this line to /usr/local/etc/profile (crew-profile-base)
trap "source ~/.bashrc" USR1

In crew, all we need to do is to send a signal to the parent shell:

Process.kill('USR1', Process.ppid)

[satmandu]

# Add this line to /usr/local/etc/profile (crew-profile-base)
trap "source ~/.bashrc" USR1

Don't we sometimes source /usr/local/etc/profile from ~/.bashrc? This would create a loop...

[supechicken]

# Add this line to /usr/local/etc/profile (crew-profile-base)
trap "source ~/.bashrc" USR1

Don't we sometimes source /usr/local/etc/profile from ~/.bashrc? This would create a loop...

No, as the trap will trigger once only (and only when) after someone try to kill -USR1 the shell process, then the trap overrides itself during source ~/.bashrc

[satmandu]

@Zopolis4 How do you feel about modifying this PR to use SIGUSR1 with trap like @supechicken suggested?

[satmandu]

I got it to work!

Try this in a container:

CREW_REPO=https://github.com/satmandu/chromebrew.git CREW_BRANCH=autosource crew update ; crew upgrade -v

[satmandu]

In #9649 I'm just execing a shell at the end of the crew process if the trap isn't set... but that should only happen once, I think.

[satmandu]

(If that's ok, you should be able to merge master and use 95% of the changes here...)

[satmandu]

I would like to merge #9649 into this.

I don't think CREW_SCRIPTING is necessary the way you have implemented it, since if we use trap we can just tell the shell to reload ~/.bashrc without reloading the shell, but that needs a variable set in the parent shell that sets the trap, which is why I'm doing that inside /usr/local/etc/profile.

But I think the essence of that would be helpful in multiple places, maybe as a CREW_NONINTERACTIVE, so we could also tell non-interactive usage to just bypass waiting around or doing any fancy graphical stuff.

zsh not working ought to be a separate bug... but also, if nobody uses it... maybe we should also just mark is as broken... maybe somebody wants to take that up as their own project.

[Zopolis4]

Updated and rebased to only have the removal of source ~/.bashrc messages.

[uberhacker]

Bump crew version in lib/const.rb?

[Zopolis4]

oops that's leftover code from the old approach

[satmandu]

So this approach doesn't work, right? So do we need to just rename the print_source_bashrc line with something else since the autosource PR handles this? Maybe 's/print_source_bashrc/source_bashrc/' or 's/print_source_bashrc/autosource_bashrc/' ?

[satmandu]

Do we just want to source bashrc after every install?

[Zopolis4]

this approach does work, but @supechicken was worried about the memory costs of opening a new bash process every time crew was run.

[Zopolis4]

Is that not harder to do?
You check the latest file modification timein /usr/local/etc/env.d/, compare it with current, and done. Shouldn't be too bad. At any rate, I'm trying to avoid repeating lots of code in package files, and this is a good place to improve.

And how do you know if ~/.bashrc has been sourced yet at all?
Not really sure what you mean here.

[satmandu]

And how do you know if ~/.bashrc has been sourced yet at all?
Not really sure what you mean here.

Sometimes we want to make sure that the bashrc is sourced after the crew command, irrespective of whether files have changed in env.d (because scripts there might change some state when being run again, even if the scripts themselves haven't changed.)

Does that make sense?

[Zopolis4]

I think so? We can set it up to source after every command, but we currently don't. If we want to do it unconditionally, it shouldn't be too hard.

[satmandu]

I think there is a middle ground where we don't want the running to be unconditional. See for instance the bash scripts for distcc.

We don't need those run after every single crew command. They only need to be run after distcc is installed.

[satmandu]

(but they can be run after every shell invocation... That's fine... If run with every crew command we will essentially get logspam with every crew command.)

[satmandu]

You need this to be yes | CREW_CACHE_ENABLED=1 crew build -f packages/hello_world_chromebrew.rb to get the unit test to pass.

[Zopolis4]

Looking into this further, setting CREW_CACHE_ENABLED=1 just papers over the bug that crew build is actually broken as it attempts to install packages after they are built, despite new binaries not having been uploaded-- we probably shouldn't be silencing bugs in our unit tests...

[satmandu]

That's not a bug. That's intentional.

Installing just built packages as part of a sequence of building multiple interdependent packages is an intentional feature of the -f flag to crew build in that context.

[satmandu]

Otherwise you can't test if a just built package installs correctly.

[Zopolis4]

But that code fails by default if CREW_CACHE_ENABLED is not set.

[satmandu]

I enable have CREW_CACHE_ENABLED=1 in all of my builds...

[satmandu]

But you're right, we should have this work without that...

[Zopolis4]

Just checking I've got the right understanding here, but we now source ~/.bashrc completely automatically, right? So we're good to delete all the print_source_bashrc stuff as per this PR.