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 two issues for symbolic linking issues with Windows (EPERM: operation not permitted, scandir/symlink) #103

Closed
wants to merge 4 commits into from

Conversation

GhostfromTexas
Copy link

This solves 2 problems with Symbolic links on windows:

EPERM: operation not permitted, scandir...
EPERM: operation not permitted, symlink...

When linking the *.build/node_modules folders. This fix forces FS to create the link as a 'junction' instead of a sym-link. This fixes the folder not being created properly with latest windows updates as well as reduce restrictions requiring an elevated console in the UAC cases.

Also, even though this was the default in FS, we made sure to link the package.json as a 'file' to secure future updates and changes to the underlying FS or Windows behaviors making this non-functional.

Tested on Linux and Windows.

@GhostfromTexas
Copy link
Author

We've created an NPM module that has this fix in it specifically for those interested.

https://www.npmjs.com/package/@hewmen/serverless-plugin-typescript

@GhostfromTexas GhostfromTexas mentioned this pull request Jun 4, 2018
@tommedema
Copy link

Is this package not being maintained? There are 19 open PRs; seems like a crucial one for Typescript to be used with serverless.

@GhostfromTexas
Copy link
Author

@tommedema it doesn't look like "serverless-typescript-plugin" has been updated since Dec of 2017. Not sure what the issue is.

This is why we made our own plugin and released it with this fix for us to use as well as other people since it was such an easy solution.

@schickling is the one that has been pushing updates in the past.

@tommedema
Copy link

Thanks @GhostfromTexas - I decided to use serverless-webpack with typescript instead, since it is more maintained.

@maplion
Copy link

maplion commented Mar 28, 2019

I am getting this issue; it's disappointing that it has apparently been resolved for almost a year and is still not merged in; is anyone maintaining this repo anymore? It doesn't appear so. Is there an active fork? Is there a reasonable alternative to this plugin? How come this isn't being maintained any longer?

@JackCuthbert
Copy link
Contributor

This looks like a really important issue that we should be addressing, lots of developers will be using Windows too! See #23, #24, #89, #100, #130...

Thanks for implementing this fix @GhostfromTexas , when I have some time this weekend I'll pull this down and give it a quick test on Mac and Linux. Are you able to include only the changes in the src/index.ts file? These look like the key changes we need for merging.

As for release, I'll need to defer to @divyenduz as I'm still new to maintaining this project.

@GhostfromTexas
Copy link
Author

GhostfromTexas commented Apr 4, 2019

@JackCuthbert Yes the changes in the src/index.ts are the only ones that are needed to fix this specific issue. You see by the fs documentation the usages of the type property on the symlinkSync call defaults to file. I wanted to be explicit in the call for both directory and file to help future proof the function hence why both lines were changed.

Documentation on the symlinkSync call

This solved any symbolic link issues I had on Windows. My assumption is that something with windows or node updated and fixed a bug that allowed the type to be left undefined and still correctly link in the case of a directory. Everything worked fine with the plugin until one day it simply didn't for me or anyone else without any changes to our code-base. It was quite odd.

Once this is implemented, I will be happy to close down the side-project we made and divert users back to the original plugin. We have no desire to maintain the project on our end.

Let me know what else you need!

  • Cameron

@maplion
Copy link

maplion commented Apr 4, 2019

@JackCuthbert As a note: I tried @GhostfromTexas fix and it didn't seem to resolve my issue (I actually pulled in the "Hewman" version directly). There is a similar fix listed in #23 that I utilized which can be found here: https://github.com/EugeneDraitsev/serverless-plugin-typescript#build . It uses a separate symlink function call with a different fs library and that did the trick for me.

@JackCuthbert
Copy link
Contributor

Thanks @maplion, looks like I'll definitely need to spin this up on Windows too 😨

Do you have any more information about your specific Windows environment you can share? I'll just be trying it out on the latest Win10. We will get this sorted!

@divyenduz
Copy link
Contributor

@JackCuthbert : Thanks, merging PRs into master should trigger a release now, I have setup semantic-release. Please test this PR and see if it can be merged, I can see that README.md and package.json can use an update, re-scoping the packages to use prisma :)

I am also open to merging this one to fix it for other OSes before fixing it for windows if that takes more time.

@maplion
Copy link

maplion commented Apr 4, 2019

So for some reason that fixed my issue [https://github.com/EugeneDraitsev/serverless-plugin-typescript#build], but not my colleagues; she has the "EPERM: operation not permitted, scandir" issue still and we now have separate behavior with the same code (really annoying). I'm going to try @GhostfromTexas fix (or something related) to see if it fixes hers, but definitely having an issue still.

I'm running Windows 10 Version 1903 (OS Build 18362.1)
She is running Windows 10 Version 1809 (OS Build 17763.404)

@geocomm-jvaquerano
Copy link

like @maplion said i'm having this issue too. I would like to use the plugin. Hopefully it is fixed soon.

@maplion
Copy link

maplion commented Apr 4, 2019

I combined both fixes presented, from @GhostfromTexas and @EugeneDraitsev, but while the problem is resolved for me, it still doesn't work on my work machine or with my colleagues. I don't know if there is some anti-virus or something preventing it from working properly (I think we have whitelists for our folders, so not sure what would block it and we added group policies to allow symlinks, so we got past that issue), but I now have two different behaviors. Maybe we should have an alternative way to utilize this without symlinks for windows (sort of like the --no-bin-links flag)? Or do you have an idea of another way to solve the issue (I honestly don't know enough about what it is doing to dig into it).

@maplion
Copy link

maplion commented Apr 5, 2019

We finally figured it out; the error is extremely misleading. This issue was solved by installing the node library scandir globally with an npm install scandir -g.

JackCuthbert added a commit that referenced this pull request Apr 6, 2019
Closes #130, #23, #89, #100, #103. New PR to be opened for resolution of
further issues with Windows symlink failures.
JackCuthbert added a commit that referenced this pull request Apr 6, 2019
Closes #130, #23, #89, #100, #103. New PR to be opened for resolution of
further issues with Windows symlink failures.
@JackCuthbert
Copy link
Contributor

While my Windows 10 VM works out when it wants to finish installing for me, I'll commit @GhostfromTexas's changes from src/index.ts in master (and update the package.json graphcool reference) as these seem pretty well agreed upon to get at least part of this problem resolved.

I've also tested this on my own project in macOS + Linux and packaging/deployment still works just fine in my case.

I'll investigate what's going on with this scandir issue in a new PR soon, thanks @maplion.

JackCuthbert added a commit that referenced this pull request Apr 6, 2019
Closes #130, #23, #100, #103, #121. New PR to be opened for resolution
of further issues with Windows symlink failures.
JackCuthbert added a commit that referenced this pull request Apr 6, 2019
JackCuthbert added a commit that referenced this pull request Apr 6, 2019
@JackCuthbert
Copy link
Contributor

Thanks again @GhostfromTexas, this has been released in version 1.1.7 🎉

@GhostfromTexas
Copy link
Author

Thanks again @GhostfromTexas, this has been released in version 1.1.7 🎉

I'm a bit late to the party! Just read through all the comments since my last post (we only use github for public packages we want to release and our other company projects are elsewhere). We also don't use serverless anymore and use Terraform as our main cloud setup tool.

Glad to see this was fixed awhile ago!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants