[Fix #2407] Use Etc.getpwuid.name to retrieve user login #2408

jujugrrr · 2015-11-09T12:52:25Z

Might be a better approach than #2406 as it keeps the "per login" cache feature.
I'll close when any of the 2 PR got merged.

martinb3 · 2015-11-09T12:59:23Z

👍 #2374 broke rubocop for us, since it missed some nil handling.

bbatsov · 2015-11-09T13:48:04Z

So, this is not going to return nil ever, right?

jujugrrr · 2015-11-09T14:19:12Z

So, this is not going to return nil ever, right?

It will definitely help to fallback to some default values. Same thing for Dir.tmpdir. The original PR passed without the safeguard, so I'm just changing the used method to what seems (I'm not an expert in getting user login 😉 ) less likely to error.

If we set some default values, then we need to chose what they are. If #2406 is a better approach it's cool for me too. It seems there was no test for 88dc507 before (or even the cache_root). Not sure how to test it easily. I've giving it a try

alexdowad · 2015-11-09T14:25:17Z

Just looked at the MRI source.

Etc.getlogin is a thin wrapper around getlogin (function prototype in unistd.h), Etc.getpwuid is a thin wrapper around getpwuid (function prototype in pwd.h).

The manpage on getlogin is very vague about how it gets its information. getpwuid is clear enough -- it reads from /etc/passwd.

alexdowad · 2015-11-09T14:26:14Z

The point here is to de-duplicate, right? It doesn't really matter if the file path uses the username or something else. Why don't you use a numeric UID? That won't require reading /etc/passwd or anything.

bbatsov · 2015-11-09T14:27:26Z

Why don't you use a numeric UID? That won't require reading /etc/passwd or anything.

Fine by me.

alexdowad · 2015-11-09T14:27:51Z

You can do that using Process.uid.

bbatsov · 2015-11-09T14:44:34Z

We can also check for the username and fallback the uid if it's nil, as usernames definitely create easier to comprehend folder names. Not a biggie, but still someone might take a look at the folders.

jujugrrr · 2015-11-09T15:11:10Z

We can also check for the username and fallback the uid if it's nil, as usernames definitely create easier to comprehend folder names

It's what I've done based on #2406. I've tried to add tests too, but you probably want stuffs to be moved around or renamed.

  the cache path when we cannot find the user name
    is created within the rubocop_cache folder
  the cache path when we can find the user name
    is created within a user name folder

jujugrrr · 2015-11-09T15:32:29Z

hum it seems, it's failing on jruby...

alexdowad · 2015-11-09T15:33:32Z

You're falling back to a blank string, not to a numeric UID.

jujugrrr · 2015-11-09T15:35:18Z

You're falling back to a blank string, not to a numeric UID.

You want me to fallback to Process.uid ?

alexdowad · 2015-11-09T15:53:58Z

I got the following through e-mail:

Actually it would defeat the purpose of the cache I guess as it will change every time you run it.

Your user ID number will not change every time you run RuboCop, unless you log out and log in again using a different account.

The point of using a user ID number is that the cache will still be per-user, rather than global. Every process has a user ID, and there is little that can go wrong when retrieving it. (Unlike retrieving the user name, which as we have seen, can be problematic sometimes.)

jujugrrr · 2015-11-09T15:56:53Z

Your user ID number will not change every time you run RuboCop, unless you log out and log in again using a different account.

yep, my bad that's why I removed the comment 😉 . I read it quickly in my head and it sounded like pid.

jujugrrr · 2015-11-09T16:05:44Z

I've pushed the fix.

masarakki · 2015-11-09T21:50:23Z

👍

bbatsov · 2015-11-10T08:11:25Z

@jujugrrr ping :-)

jujugrrr · 2015-11-10T09:24:38Z

@bbatsov had suggested that a username is desirable when it is available... could you use Etc.getpwuid.name, and fall back to Process.uid.to_s if that is nil or empty?

Etc.getpwuid.name failed the test for jruby implementation, I've never used Jruby, Maybe using getlogin then falling back to Process.uid is enough and compatible with all the platforms.

@jujugrrr ping :-)

@bbatsov Please comment above and I'm happy to submit a latest PR 😉

jujugrrr · 2015-11-10T11:20:54Z

continuous-integration/travis-ci/pr

Gem::InstallError: tins requires Ruby version >= 2.0.
An error occurred while installing tins (1.7.0), and Bundler cannot continue.
Make sure that `gem install tins -v '1.7.0'` succeeds before bundling.

It seems tins new release (10th November) is not compatible with ruby 1.9.
I've pinned it in the gemspec.

No idea on how to fix the code coverage getting lower as I've added more tests...

alexdowad · 2015-11-10T11:24:53Z

Forget about the code coverage getting lower. Coveralls is the bane of this project.

jujugrrr · 2015-11-10T11:39:14Z

issues on jruby. Not sure how the original change passed

@alexdowad I let you investigate

bbatsov · 2015-11-10T11:58:39Z

I don't even know what tins is.

alexdowad · 2015-11-10T12:20:53Z

I don't even know what tins is.

It's a dependency of my favorite gem, coveralls!

alexdowad · 2015-11-10T12:22:23Z

I suggest that we just use a numeric UID. All of this messing around just for making the name of a temp directory look pretty is not worth it.

bbatsov · 2015-11-10T12:35:44Z

Fair enough.

I'd also appreciate an alternative of coveralls, so if someone can I find one I'm instantly getting rid of this junk.

jujugrrr · 2015-11-10T13:29:33Z

I suggest that we just use a numeric UID. All of this messing around just for making the name of a temp directory look pretty is not worth it.

Fair enough

Fair enough.

I would suggest to review 3 things based on this PR though :

how did this change get in in the first place: If you wanted test and handle different cases, then the first PR should not have been merged
how to deal with breaking changes : It's a breaking change for a lot of people using Jenkins or other CI environment (gitlab). By delaying rollback/fix you are forcing people to pin down to an old rubocop version
how to deal with contributors : I've redone the PR 5 times because you changed your mind on the approach to follow and because you are asking for a much higher quality than the origin PR (I had to implement test for a method which has never been tested).

My feedback is, most people will be reluctant to contribute again to the project after this kind of experience. Which is the shame 😉. I let the branch there, feel free to use it or not.

If a builder executes the `rubocop` command with the latest version the job fails with a `no implicit conversion of nil into String` error. This is a known issue with this version and has a fix pending: rubocop/rubocop#2407 rubocop/rubocop#2408 The easiest delivery-truck workaround is to ensure $USER is set in the subprocess that executes the linting.

bbatsov · 2015-11-10T15:21:15Z

how did this change get in in the first place: If you wanted test and handle different cases, then the first PR should not have been merged

Obviously we didn't envision this would ever return nil. That's just a fact of life - errors do happen. :-)

how to deal with breaking changes : It's a breaking change for a lot of people using Jenkins or other CI environment (gitlab). By delaying rollback/fix you are forcing people to pin down to an old rubocop version

Same here - this wasn't supposed to be a breaking change. It's regrettable that this happened, but RuboCop is not my life and I can't start jumping through hoops, so that everyone would be happy. I've invested an enormous amount of personal time in the project, so I really don't feel that I owe anything more to the users. I want to solve issues as fast as possible, but this is not always possible.

how to deal with contributors : I've redone the PR 5 times because you changed your mind on the approach to follow and because you are asking for a much higher quality than the origin PR (I had to implement test for a method which has never been tested).

That's just how open-source works - ideas get discussed, implemented, rethought. I don't see the problem with that. I definitely don't want to waste anyone's time, but I'm not always certain what be best course of action is until some ideas have been brought to the table. Everyone who has contributed to RuboCop knows that I'm pretty rigid and hard to please at times and still issues do manage to squeeze in. Imagine what would have happened otherwise. Let's just reopen this and wrap it.

bbatsov · 2015-11-10T15:21:51Z

lib/rubocop/result_cache.rb

@@ -79,7 +79,12 @@ def self.cleanup(config_store, verbose, cache_root = nil)

    def self.cache_root(config_store)
      root = config_store.for('.')['AllCops']['CacheRootDirectory']
-      root = File.join(Dir.tmpdir, Etc.getlogin) if root == '/tmp'
+      if Etc.getlogin.nil? || Etc.getlogin.empty?


Let's just simplify this to using the UID and be done with it. We've wasted enough time already.

bbatsov · 2015-11-10T15:23:12Z

I'm very thankful to every contributor and I most certainly appreciate their time and desire to help the project!

jujugrrr · 2015-11-10T15:53:45Z

I'm very thankful to every contributor and I most certainly appreciate their time and desire to help the project!

hehe I know, it's just to make sure they have a good experience(or not a bad one). which is different from appreciating what they are doing.

alexdowad · 2015-11-10T16:06:36Z

Thanks @jujugrrr!

…implicity

jujugrrr · 2015-11-10T16:12:23Z

Here it is

[Fix #2407] Use Etc.getpwuid.name to retrieve user login

bbatsov · 2015-11-10T17:07:17Z

👍

bbatsov · 2015-11-10T17:09:31Z

0.35.1 is out.

alexdowad · 2015-11-10T18:02:00Z

Thanks @bbatsov!

deivid-rodriguez · 2015-11-10T20:44:07Z

I'd also appreciate an alternative of coveralls, so if someone can I find one I'm instantly getting rid of this junk.

I use codeclimate-test-reporter myself, which has a smaller set of dependencies:

coveralls (0.8.5)
  json (~> 1.8)
  rest-client (>= 1.6.8, < 2)
   simplecov (~> 0.10.0)
   term-ansicolor (~> 1.3)
   thor (~> 0.19.

vs

codeclimate-test-reporter (0.4.8)
   simplecov (>= 0.7.1, < 1.0.0)

Follow up #2408

jujugrrr force-pushed the safer_login_retrieval branch from eb14448 to ec1a7ff Compare November 9, 2015 12:54

martinb3 mentioned this pull request Nov 9, 2015

Add $username to default /tmp folder path. #2374

Merged

jujugrrr force-pushed the safer_login_retrieval branch from ec1a7ff to 79fdc75 Compare November 9, 2015 14:23

jujugrrr force-pushed the safer_login_retrieval branch from 79fdc75 to e779ef0 Compare November 9, 2015 15:07

jujugrrr force-pushed the safer_login_retrieval branch from e779ef0 to 7e122fa Compare November 9, 2015 15:45

jujugrrr force-pushed the safer_login_retrieval branch 3 times, most recently from 5015e57 to 0ca5d8f Compare November 9, 2015 16:04

masarakki mentioned this pull request Nov 9, 2015

fix bug when Etc.login is nil #2406

Closed

jujugrrr force-pushed the safer_login_retrieval branch 2 times, most recently from 3c35302 to ec78dc9 Compare November 10, 2015 10:42

jujugrrr closed this Nov 10, 2015

bbatsov reopened this Nov 10, 2015

bbatsov reviewed Nov 10, 2015
View reviewed changes

jujugrrr added 2 commits November 10, 2015 16:10

[Fix rubocop#2407] Use Process.uid rather than Etc.getlogin for s…

5fe7858

…implicity

Pinned down tins due to lack of compatibility with ruby 1.9

24c04b3

jujugrrr force-pushed the safer_login_retrieval branch from c3fb6cd to 24c04b3 Compare November 10, 2015 16:11

bbatsov added a commit that referenced this pull request Nov 10, 2015

Merge pull request #2408 from jujugrrr/safer_login_retrieval

888a67a

[Fix #2407] Use Etc.getpwuid.name to retrieve user login

bbatsov merged commit 888a67a into rubocop:master Nov 10, 2015

jujugrrr deleted the safer_login_retrieval branch November 11, 2015 08:22

deivid-rodriguez mentioned this pull request Nov 11, 2015

Remove coveralls #2420

Merged

koic added a commit that referenced this pull request Nov 20, 2023

Remove useless require 'etc'

7449316

Follow up #2408

[Fix #2407] Use Etc.getpwuid.name to retrieve user login #2408

[Fix #2407] Use Etc.getpwuid.name to retrieve user login #2408

Conversation

jujugrrr commented Nov 9, 2015

martinb3 commented Nov 9, 2015

bbatsov commented Nov 9, 2015

jujugrrr commented Nov 9, 2015

alexdowad commented Nov 9, 2015

alexdowad commented Nov 9, 2015

bbatsov commented Nov 9, 2015

alexdowad commented Nov 9, 2015

bbatsov commented Nov 9, 2015

jujugrrr commented Nov 9, 2015

jujugrrr commented Nov 9, 2015

alexdowad commented Nov 9, 2015

jujugrrr commented Nov 9, 2015

alexdowad commented Nov 9, 2015

jujugrrr commented Nov 9, 2015

jujugrrr commented Nov 9, 2015

masarakki commented Nov 9, 2015

bbatsov commented Nov 10, 2015

jujugrrr commented Nov 10, 2015

jujugrrr commented Nov 10, 2015

alexdowad commented Nov 10, 2015

jujugrrr commented Nov 10, 2015

bbatsov commented Nov 10, 2015

alexdowad commented Nov 10, 2015

alexdowad commented Nov 10, 2015

bbatsov commented Nov 10, 2015

jujugrrr commented Nov 10, 2015

bbatsov commented Nov 10, 2015

bbatsov Nov 10, 2015

Choose a reason for hiding this comment

jujugrrr Nov 10, 2015

Choose a reason for hiding this comment

bbatsov commented Nov 10, 2015

jujugrrr commented Nov 10, 2015

alexdowad commented Nov 10, 2015

jujugrrr commented Nov 10, 2015

bbatsov commented Nov 10, 2015

bbatsov commented Nov 10, 2015

alexdowad commented Nov 10, 2015

deivid-rodriguez commented Nov 10, 2015