Skip to content
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: removed process.getuid method as its not supported in winos #966

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@anikethsaha
Copy link
Member

commented Jun 19, 2019

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?
NA

If relevant, did you update the documentation?
NA

Summary
fixes #962

Does this PR introduce a breaking change?
No

Other information
So here is the thing
As far as I looked, windows seem to give users sid for a user and I guess there is no support for that to check with any file/folders as they don't keep that info with it
(maybe ! please comment if I am wrong about it)
So there is no way to check them with the file. the sid looks weird too.

H:\testing>WMIC useraccount get sid
SID
S-1-5-XX-352825XXXX-XXXXXXXXX-XXXXXXXXX-XXX
S-1-5-XX-352825XXXX-XXXXXXXXX-XXXXXXXXX-XXX
S-1-5-XX-352825XXXX-XXXXXXXXX-XXXXXXXXX-XXX
S-1-5-XX-352825XXXX-XXXXXXXXX-XXXXXXXXX-XXX
S-1-5-XX-352825XXXX-XXXXXXXXX-XXXXXXXXX-XXX
S-1-5-XX-352825XXXX-XXXXXXXXX-XXXXXXXXX-XXX

Also in windows as far as I know statSync("anyfile-or-folder").uid will always be 0
So I think removing that check seems fare or can add try-catch, at least it will be there for OSX users

try {
    if(fileOwnerId === process.getuid()){
        // actions code here , setting the utime
    }
} catch (error) {
    // setting the utime -- here
}

or no need for try-catch cause any way the timestamp needs to update in that file.

what say @evenstensberg @ematipico

@webpack-bot

This comment has been minimized.

Copy link

commented Jun 19, 2019

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@rseeberg

This comment has been minimized.

Copy link

commented Jun 20, 2019

This pullrequest should not be approved.
While it does fix the Windows monday issue by removing the fileowner check,
it reintroduces the original issue that @pastak fixed for linux & mac
in de41351

The main problem is that if you try to update a file that you are not the owner of,
you get an error.

I don't think there's any great way of getting the fileowner (or process owner) in node on windows.
You could probably wire something up in powershell, but spawning a child_process in order to update a timestamp to control a donation message.. ludicrous!

As a user of your software, i would prefer you just print the message on every build.
Then you can remove all of this logic. it might even speed up our builds ever so slightly, by not having to do SYSTEM/IO look ups

@anikethsaha

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

I would prefer you just print the message on every build.

This is actually very disturbing for some users as many others may be using webpack-cli as their dependency and if they install that package this message will pop up every time. The Current approach is the workaround to save that.

as far as de41351 is concerned I will refactor the current PR to check for the file-owner when its mac or Linux and to skip it when its windows.

@rseeberg

This comment has been minimized.

Copy link

commented Jun 20, 2019

@anikethsaha
Looking better now 👍

Still, for windows users, you can get the EPERM issue that @pastak originally attempted to fix.
This happens because you try to update the utime on a file you are not the owner of.
Since you have no way of checking if it will work or not, I don't see any way around wrapping the
if (!e) utimesSync(openCollectivePath, now, now);
in a try catch
if(!e) try { utimesSync(openCollectivePath, now, now); } catch() { /*utime update failed due to permission issues */ }

@anikethsaha

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

yeah true! thats why I dropped the idea for try-catch , I guess for windows we cant get any check for uid for file cause I guess windows doesnt keep any info on its file or folder I think

@evenstensberg
Copy link
Member

left a comment

Let's do the try catch thing described in the issue

@anikethsaha

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

windows check in catch ?

try {
    if (!e && fileOwnerId === process.getuid())
        utimesSync(openCollectivePath, now, now);
} catch (error) {
    if (!e)
        utimesSync(openCollectivePath, now, now);

}
@rseeberg

This comment has been minimized.

Copy link

commented Jun 20, 2019

Let's do the try catch thing described in the issue

Do keep in mind though, that this will mean people with permission issues, will see the donation message every time they build, until they fix their permission issues. Personally i don't mind, but..

I would prefer you just print the message on every build.

This is actually very disturbing for some users as many others may be using webpack-cli as their dependency and if they install that package this message will pop up every time. The Current approach is the workaround to save that.

as far as de41351 is concerned I will refactor the current PR to check for the file-owner when its mac or Linux and to skip it when its windows.

This could be mitigated by:

  • not showing the message
  • showing a message about why you are seeing it (due to permission issues)
  • by showing it "randomly"

const showMessage = (Math.random() * 1000) > 500;

In the end, like most things, it boils down to picking the lesser of two (three?) evils 🙂

@rseeberg

This comment has been minimized.

Copy link

commented Jun 20, 2019

windows check in catch ?

try {
    if (!e && fileOwnerId === process.getuid())
        utimesSync(openCollectivePath, now, now);
} catch (error) {
    if (!e)
        utimesSync(openCollectivePath, now, now);

}
if (!e) {
	if ((process.platform === "darwin" || process.platform === "linux") && stat.uid === process.getuid()) {
		utimesSync(openCollectivePath, now, now);
	} else {
		try {
			utimesSync(openCollectivePath, now, now);
		} catch(error) {
			/*
			 * Unable to update utime.
			 */
		}
	}
}
@anikethsaha

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

why utimesSync(openCollectivePath, now, now); inside try....that stmt gonna run anyway
I guess the condition should be in it
this alone

fileOwnerId === process.getuid()
@rseeberg

This comment has been minimized.

Copy link

commented Jun 20, 2019

why utimesSync(openCollectivePath, now, now); inside try....that stmt gonna run anyway
I guess the condition should be in it
this alone

fileOwnerId === process.getuid()

Nope, that is the issue that @pastak was fixing.
If the current process is not the owner of the file, you will NOT be allowed to update it.

It is happen EPERM when node_modules is owned by root and webpack-cli is executed by user excepted root.

@anikethsaha

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

but the code you mentioned

// ----- this one
if (!e) {
   if ((process.platform === "darwin" || process.platform === "linux") && stat.uid === process.getuid()) {
   	utimesSync(openCollectivePath, now, now);
   } else {
   	try {
   		utimesSync(openCollectivePath, now, now);
   	} catch(error) {
   		/*
   		 * Unable to update utime.
   		 */
   	}
   }
}

will update the time in no matter what with the process owner in windows
!

@rseeberg

This comment has been minimized.

Copy link

commented Jun 20, 2019

🤷‍♂ I'm not too much into node etc.
All I can say is that I have had plenty of eperm issues,
and @pastak believes they are caused by this.
Which I have no reason to doubt.
But it's more than possible you are correct.

@anikethsaha

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

I got your issue and really thankful for you to point out that. It seems I am a bit confused about what to catch in it.
@evenstensberg any help !.

Adding fileOwnerId === process.getuid() in try will not be effective as it will go to catch and if we put update time method in the catch statement for windows then it will run in mac also when the permission won't be satisfying

I think we should leave the try-catch

PS:
If the permission issues are in windows then it seems only possible way to get rid of them is to run the terminal in admin mode.
And so if a user is not having permission, then this code should simply throw a nice message, it should not update the time. that's the flow I guess it should have.

So if it is mac or linux, check the permission of it. and if it is windows, log a message or
just update the time. I guess windows wont mind that 😁

@evenstensberg

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Adding fileOwnerId === process.getuid() in try will not be effective as it will go to catch and if we put update time method in the catch statement for windows then it will run in mac also when the permission won't be satisfying

It will, it will be a gracefully done exit if perm is an issue.

And so if a user is not having permission, then this code should simply throw a nice message, it should not update the time. that's the flow I guess it should have.

Let's just try to check if the api is accessible and silent exit the script if not please

@anikethsaha

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

Let's just try to check if the api is accessible and silent exit the script if not please

what about windows time update then? it will exit for windows then
cc @evenstensberg

@evenstensberg

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

This is a valid fix, but we've removed this from master now. Submitted a commit to show you how we would do it. Might add later

@evenstensberg

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

Thanks for taking the time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.