-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
✨ Adds System.login #4800
✨ Adds System.login #4800
Conversation
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.
Also, can we not use emoji in commit messages, it adds absolutely no information.
spec/std/system_spec.cr
Outdated
|
||
describe "login" do | ||
it "returns String or nil" do | ||
System.login.should be_a(String | Nil) |
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.
Please test this by comparing the result with echo $USER
or similar.
@@ -11,6 +11,7 @@ module Crystal | |||
end |
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.
Please add a commented out spec for the Crystal::System.login
method.
src/system.cr
Outdated
@@ -22,4 +22,14 @@ module System | |||
def self.cpu_count | |||
Crystal::System.cpu_count | |||
end | |||
|
|||
# Returns the short user name of the currently logged in user or `nil` if it | |||
# can't be determined. Note that this information is not secure. |
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.
Please clarify on what "secure" means here.
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.
Additionally, when can this return nil? I don't see a reason to declare it as nillable.
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.
Re: nil, there's no guarantee that getlogin
returns a string, it can return NULL. That's the source of the comment - so I'm not certain how to handle that particualr behavior.
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 is what happens when getlogin
returns a NULL
currently: https://carc.in/#/r/2hbo. You need to manually convert it to a nil
, String.new
won't do that for you.
Uses current conventions. Split out from crystal-lang#2368 Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
1af3125
to
c109496
Compare
spec/std/system_spec.cr
Outdated
it "returns String or nil" do | ||
shell_login = `echo $USER`.strip | ||
login = System.login | ||
login.should be_a(String) |
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.
We typically don't check return types in spec. If you want to be sure of the returned type, annotate the method.
src/crystal/system.cr
Outdated
@@ -7,10 +7,15 @@ module Crystal | |||
# Returns the number of logical processors available to the system. | |||
# | |||
# def self.cpu_count | |||
|
|||
# Returns the short user name of the currently logged in user. | |||
# |
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 think that the comment should be adjacent to the def self.login
as with hostname
. Please can you also modify cpu_count
at the same time, as this is a tiny change not worth a new PR.
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
spec/std/system_spec.cr
Outdated
|
||
describe "login" do | ||
it "returns String when found" do | ||
shell_login = `echo $USER`.strip |
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.
@RX14 Now that I've got the logic to return Nil
, I'm not certain how to structure this test case to handle the current CI - travis has no $USER
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.
According to travis doc there is a $USER
env var on all builds: https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables
It just states that the value may not always be travis
.
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.
$USER
should be universal to all unix systems.
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.
All true, yet the returned value of $USER inside the docker runtime on Travis returns null.
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.
No, it seems correct. $USER
is ""
or unset, and System.login
is Nil
. It seems the bug is in the spec for not equating ""
and Nil
. It also just occured to me that `echo $USER`.strip
is a weird way to get the value when we have ENV["USER"]
. Using ENV["USER"]?
would give the correct behaviour of being Nil
when unset.
The login name is Nil
because the specs are running inside a docker container - which has no login session. That brings us to another question, why is this method useful? It doesn't tell us the current effective user of the process, just the login session.
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.
@RX14 I was using your recommendation from #4800 (comment) .
I'll change to using ENV.
As to the utility, if I log in using user1
and later do sudo su - user2
, System.login will return user1
, whereas ENV["USER"]
will return user2
.
For process-specific owner lookup, an enhancement to Process
implementing the getuid
call would do what you're asking about.
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.
BTW you need ENV["USER"]?
to handle the case the env var isn't set.
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.
Thanks!
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.
Oh, just realised. `logname`.strip
is the command equivalent to getlogin
, not ENV["USER"]?
. Sorry for making you jump through hoops! You'll also need to convert "logname: no login name" to nil
to pass the test on travis.
src/crystal/system/unix/login.cr
Outdated
module Crystal::System | ||
def self.login | ||
if LibC.getlogin | ||
String.new(LibC.getlogin) |
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.
Wouldn't it be better to store the result in a local variable instead of calling LibC.getlogin
twice?
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
I think we should merge a PR to bind |
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
spec/std/system_spec.cr
Outdated
describe "login" do | ||
it "returns current session login" do | ||
shell_login = `logname`.strip | ||
shell_login = nil if shell_login.match(/no login name/) |
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.
Please check the exit status, not an arbitrary error message. POSIX specifications demand to provide a diagnostic message but it might not always be (or contain) no login name
. But logname
must return non-zero exit status if no login is available - thats easier and faster anyway.
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Happy to rebase once we're green and people are 👍 with the changeset. |
src/crystal/system/unix/login.cr
Outdated
if login = LibC.getlogin | ||
String.new(login) | ||
else | ||
nil |
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.
You don't need the else nil
branch.
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.
No, you don't. But I think it looks nicer and is clearer as to what's going on.
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.
If a condition is true then return something, otherwise nothing, aka nil
in crystal terms. Adding else nil
is just noise.
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.
It seems else nil
is used only once in the entire repo: https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/semantic/call_error.cr#L288 And this should probably be removed as well.
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Anything else for me to do here? |
@RX14 Anything remaining here, or can this be merged? |
I believe there is nothing more to do. I'm just dubious if we really need this, personally. |
I think this and a PR to access the processes' effective uid/gid should be merged at the same time so that their respective documentation can reference each other and their difference. I think that accessing the effective uid/gid will be much more common and useful than this method. |
@ysbaddaden If this isn't desirable, that's fine - let's make a decision and get on with it. 😀 This being split out from #2368 - It appeared to be desired, that's why I took it on. Feedback on the desirability of this has been spotty at best. @RX14 It is entirely possible that you're right, and that other extensions may be more useful - I didn't try to conquer those, so this remains waiting for someone to decide what its future should be. I'm happy to consider other directions, just need some feedback on what to do about this PR. |
Could this get revisited now that #5627 is getting ready to be merged? |
Closing stale PR. There has been no activity for a long time. This is probably still worth implementing, but needs a fresh start to discuss the API and integrate with existing APIs (see #7738). |
Uses current conventions.
Split out from #2368
Signed-off-by: Mike Fiedler miketheman@gmail.com