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

windows support #39

Closed
ryanflorence opened this issue Feb 25, 2014 · 42 comments
Closed

windows support #39

ryanflorence opened this issue Feb 25, 2014 · 42 comments

Comments

@ryanflorence
Copy link

I noticed the README says there's no windows support yet. Wanted to open the discussion here while broccoli is still young. What's the hold up?

@MajorBreakfast
Copy link

Node's sync file API is brittle on windows (Source) and broccoli uses it exclusively.

Discussion about async vs sync performance: joliss/node-walk-sync#1
EPERM on windows https://github.com/joliss/broccoli/issues/11

The best way to address the issue would be to go all async and switch to sync in performance critical regions on Linux and maybe Mac. Or at least that's how I currently see it. (Replacing something async with something sync and calling the callback immediatly is easy. The other way around not so much)

@joliss
Copy link
Member

joliss commented Feb 25, 2014

Node's sync file API is brittle on windows (Source) and broccoli uses it exclusively.

I don't see anything in principle stopping us from using async IO, but it seems that the issue you linked (6599) is basically a problem in Node and needs to be fixed there. Curiously, the way they're reproducing it is with concurrent read & write access, and I can't think of any place where we're doing this in Broccoli. Perhaps there's a bug in Broccoli or a plugin; or the Node issue happens in more places than 6599 suggests.

@joliss
Copy link
Member

joliss commented Feb 25, 2014

For Windows support, there's also hard linking in some plugins. It seems we need a hardlink-or-copy-and-preserve-mtime (with and without overwriting) kind of package to replace it. It's curious that we haven't run into this. Does fs.linkSync not fail on Windows?

@joliss joliss mentioned this issue Feb 25, 2014
@joliss
Copy link
Member

joliss commented Feb 25, 2014

By the way, in general, I consider Windows support necessary, but it's really really low on my list of priorities. Don't expect me to put much work into it over the next few months.

@MajorBreakfast
Copy link

Linking works fine because the build works. It is supported by the file system (msdn article) and node integrates it.

@stefanpenner
Copy link
Contributor

@joliss hopefully the community will help with windows support then. It's likely a large requirement for many.

@ebryn
Copy link
Contributor

ebryn commented Mar 10, 2014

Could someone put together a list of known issues with Windows in case someone stumbles upon this issue and has some time to invest in getting it to work?

@joliss
Copy link
Member

joliss commented Mar 11, 2014

The only specific issue I know of is

That said, I think what really needs to be done to get Broccoli working reliably on Windows is

  • writing a test suite (including perhaps some fairly high-level integration tests).

I don't personally have a dev environment on Windows around, so to stop Windows support from breaking repeatedly, having a test suite seems like a prerequisite.

@MajorBreakfast
Copy link

Instead of totally crashing, broccoli is now still operational after such an error occurs. I think it's because of the error handling with promises. We should try using rimraf async in quick temp.

@MajorBreakfast
Copy link

I created a PR for node-quick-temp that makes it async. This is the place where the calls to rimraf happen and probably the hot spot from where most windows errors originate. If that gets merged and we notify the plugin authors that they should update their code to only use async stuff, windows should be supported.

@joliss
Copy link
Member

joliss commented Mar 13, 2014

I created a PR for node-quick-temp that makes it async. This is the place where the calls to rimraf happen and probably the hot spot from where most windows errors originate.

The issue with random EPERM errors clearly needs to be fixed. I believe however that it needs to be fixed in Node - this issue you mention might be related - and Broccoli shouldn't concern itself with it. If filesystem IO has strange race conditions and is unreliable, things are fundamentally going to break, and there's nothing we can do about it. Trying to play whack-a-mole and putting in workarounds (like rimraf.js:20) as issues pop up might be convenient as a stop gap, but I believe that it's fundamentally futile. I worry that trying to maintain this kind of code is going to turn into a timesink of "just one more workaround". So I'm not willing to accept PRs to taper over such race conditions.

In my view, the proper approach to fixing this is to pop open the Node source code and figure out what's really causing the EPERM errors.

There are two exceptions:

  • The related Node issue alludes to this being triggered when files are being read and written in parallel. I don't believe that this is what's happening in this case, but if it was, there might be some kind of race condition in Broccoli, and we should fix it.
  • If it turns out theoretically impossible to do certain things on Windows, as opposed to being a bug in Node's Windows code, I'd be happy to revisit.

@jgable
Copy link

jgable commented Mar 13, 2014

Looking through the source recently and noticed a couple places with hard coded / for making paths that will likely cause problems on windows. You'll want to replace those with path.join where possible or use path.sep.

@joliss
Copy link
Member

joliss commented Mar 13, 2014

Does the '/' actually cause problems on Windows?

path.sep is a possibility but hard to read. path.join is too slow for many applications.

@jgable
Copy link

jgable commented Mar 13, 2014

I believe it will eventually cause you problems. Interesting about path.join performance, but it's there to solve multi-platform issues like this. Your approach of getting it faster in core is probably the best tack.

@MajorBreakfast
Copy link

I never had any problems with forward slashes on windows. Node's file functions can cope with them. Or at least I didn't encouter any problems so far.

@ebryn
Copy link
Contributor

ebryn commented Apr 6, 2014

@MajorBreakfast have you had any luck with advancing Windows support for Broccoli?

I'd love to see Windows support within the next couple weeks. I'd be willing to offer a bounty if necessary.

@MajorBreakfast
Copy link

@ebryn It's easy to switch to sync in an async environment. The other way around is impossible without rearchitecturing. The decision is up to @joliss. To make it happen she has to make the switch by communicating the message to all plugin authors and correcting broccoli itself.

If you ask me, then async is the way to go in run loop based systems like JavaScript. (windows bugs or not)

Windows throws errors if you're trying to alter/delete files that are currently in use (EBUSY). The async implementation of rimraf handles them through retries after timeouts. This cannot be done synchronously because timeout is async.

@joliss
Copy link
Member

joliss commented Apr 11, 2014

Just to get everyone on the same page: Windows support is, as far as I understand the issue, not blocking on making file IO in Broccoli async. That would seem to be a workaround for brokenness in Node, and my views are summarized above. As it is, I will not accept PRs to make things asynchronous, as changing the code structure to work around broken code elsewhere is clearly excessive.

I'm happy to agree to disagree with @MajorBreakfast, but wanted to be clear about what as a maintainer I will and won't merge.

(You can obviously use asynchronous code in plugins as you like - this is accomodated through promises - and this is perfectly fine. I'm not saying everything has to be synchronous. Just that using synchronous disk IO isn't bad.)

The way forward seems to be to investigate where the random EBUSY/EPERM errors are coming from, if I understand the problem correctly. It might be a problem with how Node/libuv does disk IO, or some more fundamental issue about how IO works on Windows in general.

@ebryn
Copy link
Contributor

ebryn commented Apr 28, 2014

I'd like to see this get fixed within the next few days, as I'd like to use Broccoli for my Ember.js training classes.

I'm offering a $200 bounty for a solution that @joliss is satisfied with.

@stefanpenner
Copy link
Contributor

@ebryn thoughts on a https://www.bountysource.com/ ?

@rwjblue
Copy link
Member

rwjblue commented Apr 28, 2014

I kicked things off with BoutySource.com:

https://www.bountysource.com/issues/1509493-windows-support

Lets create a bounty large enough to make it happen!

@ebryn
Copy link
Contributor

ebryn commented Apr 28, 2014

From my conversation with @joliss, I've decided to pursue interim solutions like the ones suggested by @MajorBreakfast.

We need Windows support ASAP for ember-cli. It's a massive blocker for my training classes, improving the Ember.js builds, and for everyone trying to use ember-cli on Windows. It's not pragmatic to block Windows support on fixing underlying issues in Node.js itself. It'll likely take months to get these fixes out into a stable build that our users will be able to use on their machines. That said, I believe we should be pursuing these fixes in parallel and remove the workarounds once a large population of users have upgraded to the appropriate version of Node in the future.

@ryanflorence
Copy link
Author

If we can work around the issue then we absolutely should.

  • jQuery would not be useful if it waited for browser's to fix bugs
  • (I think) maintaining and depending on forks is a bigger liability than managing the work-around

I had no idea how big windows was among ember developers until ember-tools had issues in windows.

@MajorBreakfast
Copy link

@rpflorence Developping on windows has the huge advantage that you can immediatly test in IE. (and not through a VM) It has the adobe suite, sublime text and a command line - that's all you need. And since almost all computers except Macs ship with it, it's quite popular.

@g13013
Copy link

g13013 commented Apr 28, 2014

@rpflorence totally agree

For now, I am using broccoli on windows and I didn't (for now) experienced your problem, the only one problem is with serve command raising ENOTEMPTY somtimes and I have to delete tmp folder so that it works again, my first investigations led me to think it has to do with the tmp folder not being cleaned correctly, maybe the node's exit event works differently on Windows

@krisselden
Copy link
Contributor

@joliss I truly understand the frustration with what is Node's bug but I do not think waiting is the pragmatic choice. Ember-CLI and others now are going to be forced to fork broccoli in order to support Windows and this is going to cause confusion at a time when broccoli is collecting steam.

I implore you to reconsider.

@g13013
Copy link

g13013 commented Apr 28, 2014

@joliss and WE are here to help you 👍

@rwjblue
Copy link
Member

rwjblue commented Apr 28, 2014

Can someone link me to a repo that I can run under windows that fails to build properly? We are all talking about "the windows issue", but I haven't seen a set of repro steps yet....

@stefanpenner
Copy link
Contributor

@rjackson from my understanding it is sparatic and typically manifests only under use. Maybe something we can script?

@rwjblue
Copy link
Member

rwjblue commented Apr 28, 2014

Yeah, I'll throw my Ember builds at an EC2 instance sometime this week just to see what happens.

@ebryn
Copy link
Contributor

ebryn commented Apr 28, 2014

We're working around this in ember-cli for now: ember-cli/ember-cli#493

Big thanks to @MajorBreakfast! He refused my bounty... what a guy! :)

@g13013
Copy link

g13013 commented Apr 28, 2014

Can you guys explain in which case it fails ?

@rwjblue
Copy link
Member

rwjblue commented Apr 28, 2014

Basically, no. I have yet to see anyone actually point to an issue that is actionable.

@g13013
Copy link

g13013 commented Apr 28, 2014

@rjackson there's actually an issue with the serve command as I explained above, but it not "critical" for now, still it's an issue:
https://github.com/joliss/broccoli/blob/master/lib/server.js#L19
I think that the problem can be solved by cleaning on SIGINT signal in place of exit ?
see node process events

@g13013
Copy link

g13013 commented Apr 30, 2014

@g13013
Copy link

g13013 commented May 1, 2014

🆙

@stefanpenner
Copy link
Contributor

This should be an isolated reproduction + explanation.

https://gist.github.com/stefanpenner/2cc619b8740fe2463c2a

@ryanflorence ryanflorence changed the title windows support windows support [$50] May 2, 2014
@ryanflorence ryanflorence changed the title windows support [$50] windows support May 2, 2014
@joliss joliss removed the bounty label May 2, 2014
@joliss
Copy link
Member

joliss commented May 2, 2014

I've added this on https://github.com/joliss/broccoli/issues/128.

Should we close this issue for now? If there are more problems with Windows support, we can open more issues for them.

@stefanpenner
Copy link
Contributor

@joliss ya sounds good

@joliss joliss closed this as completed May 2, 2014
@joliss
Copy link
Member

joliss commented May 3, 2014

I just merged @krisselden's fix, PR #130, and released Broccoli 0.10.0. Can people please try buildling and serving broccoli-sample-app on Windows and let me know if it works now?

@joliss
Copy link
Member

joliss commented May 5, 2014

Note: Some of the remaining issues may be due to virus scanners interfering with the files. Can people please try it, and if there's issues, report if they happen with or without virus scanners on?

@g13013
Copy link

g13013 commented May 5, 2014

yeah, it might be!

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

No branches or pull requests

9 participants