-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Fix #2407] Use Etc.getpwuid.name to retrieve user login #2408
Conversation
eb14448
to
ec1a7ff
Compare
👍 #2374 broke rubocop for us, since it missed some nil handling. |
So, this is not going to return |
It will definitely help to fallback to some default values. Same thing for 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 |
ec1a7ff
to
79fdc75
Compare
Just looked at the MRI source.
The manpage on |
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 |
Fine by me. |
You can do that using |
We can also check for the username and fallback the uid if it's |
79fdc75
to
e779ef0
Compare
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.
|
hum it seems, it's failing on jruby... |
You're falling back to a blank string, not to a numeric UID. |
You want me to fallback to |
e779ef0
to
7e122fa
Compare
I got the following through e-mail:
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.) |
yep, my bad that's why I removed the comment 😉 . I read it quickly in my head and it sounded like pid. |
5015e57
to
0ca5d8f
Compare
I've pushed the fix. |
👍 |
@jujugrrr ping :-) |
@bbatsov Please comment above and I'm happy to submit a latest PR 😉 |
3c35302
to
ec78dc9
Compare
It seems No idea on how to fix the code coverage getting lower as I've added more tests... |
Forget about the code coverage getting lower. Coveralls is the bane of this project. |
issues on jruby. Not sure how the original change passed @alexdowad I let you investigate |
I don't even know what |
It's a dependency of my favorite gem, coveralls! |
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. I'd also appreciate an alternative of coveralls, so if someone can I find one I'm instantly getting rid of this junk. |
Fair enough. I would suggest to review 3 things based on this PR though :
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.
Obviously we didn't envision this would ever return
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.
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. |
@@ -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? |
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.
Let's just simplify this to using the UID and be done with it. We've wasted enough time already.
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.
sure
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. |
Thanks @jujugrrr! |
c3fb6cd
to
24c04b3
Compare
Here it is |
[Fix #2407] Use Etc.getpwuid.name to retrieve user login
👍 |
0.35.1 is out. |
Thanks @bbatsov! |
I use
vs
|
#2407.
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.