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

distrobox-export: add login shell option #651

Merged
merged 3 commits into from Mar 9, 2023

Conversation

Scafir
Copy link
Contributor

@Scafir Scafir commented Mar 8, 2023

Add the option (-l or --login) to distrobox-export so that the exported command runs inside a login shell, giving it access to the variables sourced during login (e.g: /etc/profile).
If --sudo is also specified, then care has been taken so that it also runs in a login shell (via -i).

Fixes #639

Add the option (-l or --login) to distrobox export so that the exported
command runs inside a login shell, giving it access to the variables
sourced during login (e.g: /etc/profile).
If sudo is also specified, then care has been taken so that it also runs
in a login shell (via -i).
distrobox-export Fixed Show fixed Hide fixed
distrobox-export Fixed Show fixed Hide fixed
distrobox-export Fixed Show fixed Hide fixed
distrobox-export Fixed Show fixed Hide fixed
distrobox-export Fixed Show fixed Hide fixed
distrobox-export Fixed Show fixed Hide fixed
@89luca89
Copy link
Owner

89luca89 commented Mar 9, 2023

Hi @Scafir thanks for the PR!

Just a couple of comments there, they refer to the use of "${variable}" instead of $variable

I think after fixing those, and running CI, we can merge

Signed-off-by: Luca Di Maio <luca.dimaio1@gmail.com>
Signed-off-by: Luca Di Maio <luca.dimaio1@gmail.com>
@89luca89 89luca89 added the CI label Mar 9, 2023
@89luca89
Copy link
Owner

89luca89 commented Mar 9, 2023

All green, we can merge :)
Thanks a lot!

@89luca89 89luca89 merged commit 5360ade into 89luca89:main Mar 9, 2023
137 checks passed
if [ "${is_login}" -ne 0 ] && [ "${is_sudo}" -ne 0 ]; then
start_shell="sudo -i"
elif [ "${is_login}" -ne 0 ]; then
start_shell="sh -l -c"

Choose a reason for hiding this comment

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

This does not work for commands taking arguments since -c takes a string, and not arbitrary parameters. Also shouldn't it use the user shell rather than sh

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks @fredizzimo I've pushed a fix for it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, the fix does not seem to work, as for some reason, sudo prevents my user to run as himself:

Sorry, user XXX is not allowed to execute 'YYY' as XXX on ZZZ

Maybe the sudo -u ${USER} -i could be replaced by ${SHELL} -l?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking a bit further, there is another issue that I don't quite understand. If you run bash gcc, it will abort with cannot execute binary file, whereas bash -c "gcc" successfully runs. It seems to me that having a structure like this:

${SHELL} -l -c "exportedCommand $@"

is the most general solution.

Choose a reason for hiding this comment

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

In order to work around the issues I had here #656 I made changes to always run the login shell, no matter what. In distrobox-enter

        result_command="${result_command} ${container_name}"
 
        if [ -n "${container_command}" ]; then
-               result_command="${result_command} ${container_command}"
+                quoted_container_command="\\\"${container_command}\\\""
+               result_command="${result_command} sh -c \"\\\$(getent passwd ${USER} | cut -f 7 -d :) -lc ${quoted_con
tainer_command}\""
        else
                # if no command was specified, let's execute a command that will find
                # and run the default shell for the user

So that's basically what's @Scafir is suggesting to do here, allthough the quoting needed special handling there. I'm not sure if it's needed here as well.

By always running the login shell I can guarantee that the rc file is always run and I can unset and set the environment variables I want. This variant has been working well for me, but I'm not sure if that's the best way to solve the issues, so I have not sent any PR .But anyway I think a similar approach could be taken here as well.

89luca89 added a commit that referenced this pull request Mar 19, 2023
Signed-off-by: Luca Di Maio <luca.dimaio1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Suggestion] distrobox-export: run in login shell
3 participants