-
Notifications
You must be signed in to change notification settings - Fork 239
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
[6.0-marshmallow branch] add gosu; refactor things #31
base: 6.0-marshmallow
Are you sure you want to change the base?
Conversation
|
074f7a1
to
3d40e47
Compare
@@ -3,55 +3,47 @@ | |||
# | |||
FROM ubuntu:14.04 | |||
|
|||
MAINTAINER Kyle Manna <[email protected]> | |||
LABEL maintainer "Kyle Manna <[email protected]>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The preferred way to specify this metadata is via a LABEL
dpkg -i *.deb && \ | ||
apt-get -f install && \ | ||
apt-get clean && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not needed anymore...Java 7 is used.
9f66bec
to
a7e4d70
Compare
f7b4b2f
to
05fad9b
Compare
c8a6110
to
d1a5a03
Compare
Fixed a small issue and rebased |
utils/docker_entrypoint.sh
Outdated
|
||
# Execute command as `aosp` user | ||
export HOME=/home/aosp | ||
exec sudo -u aosp $args | ||
exec gosu aosp ${args:-"bash"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not change this to just sudo -E <cmd>
instead of all the gosu install?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure...sudo in docker is a disaster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main reason is that it just looks cleaner from the process perspective...run top
and you'll see...I was having issues with the environment variable for ccache not being seen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to see the reason:
With sudo -E:
aosp@c1b4674f2f9c:/aosp$ ps waxu
USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
root 1 1.0 0.0 47132 3152 pts/0 Ss 00:18 0:00 sudo -u aosp -E bash
aosp 18 0.0 0.0 18172 3200 pts/0 S 00:18 0:00 bash
aosp 21 0.0 0.0 15580 2040 pts/0 R+ 00:18 0:00 ps waxu
aosp@c1b4674f2f9c:/aosp$
Now with gosu:
aosp@7e6014c412b9:/aosp$ ps waxu
USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
aosp 1 1.7 0.0 18172 3316 pts/0 Ss 00:16 0:00 bash
aosp 24 0.0 0.0 15580 2040 pts/0 R+ 00:16 0:00 ps waxu
aosp@7e6014c412b9:/aosp$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This explains it concisely but I've pretty much shown what they were demonstrating.
you jus see the process, not a forked process |
that said, hold off on merging this...wanna see if I can fix a small issue...there's a weird issue where I'm seeing an unmet dependency error related to openjdk and wanna fix that |
8f176e0
to
1460c4b
Compare
@kylemanna -- this is ready for showtime -- I think we should use gosu -- the rationale behind why it exists is good -- as well as the fact that it just gets out of the way vs what Ultimately -- this up to you -- I'm just contributing back my changes :) |
51ddf21
to
4c49921
Compare
2179bf7
to
d408a02
Compare
This will also fix issues with the ccache env vars not showing up -- at least in my testing.