Check for sudo before using it with package managers #300
Conversation
|
Personally, I don't think we should use this to gate |
|
@staticfloat Something like this? (See second commit) |
|
Yeah, something like that should probably work. We'll need to initialize the |
|
Okay, I've now made it so that if the user does not explicitly pass a |
|
Yeah that looks good. Have you tested this on any platforms? |
|
I don't have Linux handy, and so far no packages are set up to use the |
|
|
Aha! It took me a while to realize what was going on here. You need to initialize Other than that, this works! |
|
What environments have package managers but not sudo? I'm confused by how that could ever happen |
|
This is the case with all (default) |
|
docker suggests that you should run things as an unpriviliged user within the container, and for security reasons we should do so on the buildbots. but guess this should be mostly harmless |
|
We do run as an unprivileged user on the buildbots, so even with this, the
linked jlbuild invocation won't work. :P
-E
…On Thu, May 25, 2017 at 5:59 PM, Tony Kelman ***@***.***> wrote:
docker suggests that you should run things as an unpriviliged user within
the container, and for security reasons we should do so on the buildbots.
but guess this should be mostly harmless
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#300 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAH_aJV3ieyPR4UH1ED503AIgeU8BO3Rks5r9iP-gaJpZM4NkpTE>
.
|
|
so this would only help if you happen to be running |
|
I don't think I understand why this wouldn't address the issue in the linked invocation of jlbuild |
This is what happens if you
Because on the buildworkers you currently can't install new packages. There is no sudo and you're running as a non-root user. I may change this eventually (add passwordless sudo) but at the moment, it is what it is. ;) |
|
Is this even useful then? |
|
Yes, I still think this is a good idea. |
Codecov Report
@@ Coverage Diff @@
## master #300 +/- ##
=========================================
- Coverage 36.89% 36.69% -0.2%
=========================================
Files 4 4
Lines 805 812 +7
=========================================
+ Hits 297 298 +1
- Misses 508 514 +6
Continue to review full report at Codecov.
|
would be better to only allow package installation privileges for the buildbot non-root user, if that's doable |
|
Bump |
|
|
||
| # Definitions that should be evaluated upon initialization, not stored in the cache | ||
| function __init__() | ||
| global const has_sudo = try success(`sudo -V`) catch err false end |
There was a problem hiding this comment.
This shouldn't be a global const which can cause issues when used in __init__ for precompilation
There was a problem hiding this comment.
Shouldn't matter in this case.
There was a problem hiding this comment.
global const is incompatible with precompilation
There was a problem hiding this comment.
| @@ -356,6 +356,7 @@ function provides{T}(::Type{T},providers::Dict; opts...) | |||
| end | |||
| end | |||
|
|
|||
| sudoname(c::Cmd) = c == `` ? "" : "sudo " | |||
There was a problem hiding this comment.
I don't understand why include the space after sudo ?
There was a problem hiding this comment.
So it looks nicer when it's interpolated below.
There was a problem hiding this comment.
how so ? looks like it could be confusing, I wouldn't expect the sudo name to include a space after
This addresses an issue noticed in JuliaLang/julia#21956, where Yum is available so BinDeps tries to use it, but the installation uses
sudo, which is not installed on the buildbots. With this PR, binary package managers are only usable ifsudois available.