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: first attempt #224

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yeputons
Copy link
Contributor

@yeputons yeputons commented Jan 7, 2014

Hello everybody out there using trying to use Meteorite on Windows

I've tried to start Meteorite on Windows and, not surprisingly, it didn't work at all. My computer's background: npm, msys, meteor and git are installed (everything is in the PATH). I use half-official meteor for Windows, the one from 'bootstrap script', not MSI.

I've investigated the problem a little and I found three things that prevent mrt install/mrt update from running correctly on my system:

  1. git clone do not create several nested folders, as it do on Linux. Well-known, see Git source dependencies don't work on Windows sbt/sbt#895 .
  2. meteor on Windows is not an executable, it's a script called meteor.bat - big difference, as only executables can be ran directly, in contrast with linux. So, usual spawn fails to find meteor.exe. Well-known too - child_process.spawn ignores PATHEXT on Windows nodejs/node-v0.x-archive#2318
  3. Unlink behaves differently on Windows and Linux with read-only files ( fs.unlink() treats permissions differently on Windows vs. Unix nodejs/node-v0.x-archive#3006 ). On Linux, they're normally deleted. On Windows, function fails with ENOPERM.

I've fixed all of them locally and reported number three to wrench-js: ryanmcgrath/wrench-js#70 .

This branch with fixed wrench-js (as I've suggested in the last issue) works OK on Windows. At least, I can perform mrt install, mrt update and mrt create.

It would be great to get some feedback from another Windows users and to get thoughts of Meteorite authors on this issue.

@yeputons
Copy link
Contributor Author

yeputons commented Jan 8, 2014

By the way, I've found another attempt of making Meteorite work on Windows by @seanmwalker : seanmwalker@da6190d I think some code may be taken from that repository.

@seanmwalker
Copy link

Hi Egor,

Please let me know if you find any issues with this fork of meteorite. I'm preparing to make a pull request, as I've not had anyone report any issues with it yet. But I'll be happy to help if you find anything. One quick note: Unless you put meteorite in the beginning of your windows path the command line still finds the Microsoft virus scanner software first. This has not been an issue in a cygwin bash shell, and works as as one would expect.

Thanks, looking forward to any feedback for changes before the pull request.

@tmeasday
Copy link
Member

tmeasday commented Jan 8, 2014

Great stuff!

Seems very reasonable to me.

You should be able to run the tests (on *nix at least -- obviously ideally they'd work on Windows too, although that may present other problems) -- I can run them here later today if you don't have a machine to try them on.

@yeputons
Copy link
Contributor Author

yeputons commented Jan 8, 2014

@tmeasday Unfortunatelly, I am unable to run tests even on Debian i686. Log, I use node v0.11.8-pre, Npm 1.3.12, Meteorite from checkout. I've also cloned https://github.com/oortcloud/atmosphere/ , ran it on port 3333 and imported bson from data/ into local mongo (and got list of 303 packages).

@tmeasday
Copy link
Member

tmeasday commented Jan 9, 2014

Hey @yeputons - Probably some better documentation is needed. Some notes:

  1. You don't need anything in your local atmosphere, in fact, it's better if you clear it out.
  2. You may need to run git submodule update --init -- mrt-test-pkg1 refers to a package which the tests use.

@tmeasday
Copy link
Member

tmeasday commented Jan 9, 2014

P.S. if you are still having trouble, let's organise a time to meet on IRC or elsewhere to try and get it going?

@gdennie
Copy link

gdennie commented Jan 9, 2014

To avoid conflict with the existing Microsoft's "malicious software removal tool" (mrt.exe), why not add a meta-command that then invokes meteorite's mrt and name it something like, "wmrt" or "meteorite". If you use a batch file to implement this, it might be...

wmrt.bat
/path/../mrt %1 %2 %3 ... %9

Then...
wmrt parm1 parm2 ... parm9

@tmeasday
Copy link
Member

Hey all, I updated Meteorite slightly and made the documentation more helpful: https://github.com/oortcloud/meteorite/blob/master/CONTRIBUTING.md#testing

I'm having running the tests right now as github appears to be flaky, but theoretically it should work.

@seanmwalker
Copy link

I now have the tests running and passing using the meteorite/master branch on a Ubuntu VM. The plan is to merge master with my fork to see if there are any issues created on Linux, and to remedy any tests that will fail on Windows. @yeputons would you (and/or any others) be interested in combining efforts to get a meteorite release candidate for Windows using one of our forks or a fresh one?

@yeputons
Copy link
Contributor Author

@seanmwalker sure, I'm really interested in getting Meteorite on Windows. Here's my thoughts on it:

  1. I don't think that explicitly append extension to 'meteor' is a good idea, because it can change in the future. I like my approach more, because it not only heals this problem but also prevents possible problems with another applications, like git. I, as a Windows user, don't like when something work with .exe and doesn't work with .cmd. Of course, we should test that 'spawn-cmd' works on anoter systems as well. If it's OK, we don't even have to process Windows separately.
  2. I like your idea with symlinks replaced by junction. It whould be done, but junctions may be dangerous when used with explorer - it may not recognize that it's a link and recursively remove all content from it while removing 'packages' directory, which is not cool - removal of one project shouldn't affect another. I think further investigation about junctions and symlinks in different versions of Windows is required.
  3. What's about recursive directory creation, wrench has this functionality, both of us haven't used it somewhy: wrench.mkdirSyncRecursive . I think it should be used.
  4. Do you have problem with unlink? It's very deep node issue, so may be some workaround would be useful.
  5. Some synonym should also be added. I prefer ''meteorite', but it can also be 'wmrt' as suggested above.
  6. We can use any branch as a base. But I would like to have the ability to commit to it without pull requests - does GitHub provide us an option to open commit access to another users?

Wait for your thoughts about it. I think our nearest goal is to choose a single approach on all these points.

@tmeasday
Copy link
Member

Hey guys, I'll go ahead and give you write access to the repo so you can make a windows branch and push to that if it makes sense for point 6.

@yeputons
Copy link
Contributor Author

@tmeasday Thanks.

Everyone, I found one very fun issue with my approach and spawn-cmd: Windows has .js in PATHEXT by default, which causes it to call mrt.js with standard script handler (not Node), when mrt is called from the bin/ folder, which is what happening during tests. Not cool.

Another problem: runner.invokeMrt assumes that Meteorite executable is one file, which is not true on Windows, because Node's executable should be explicitly specified. Possible solution is to create a bat file, similar to what Node on Windows does during npm link/npm install).

@yeputons
Copy link
Contributor Author

@tmeasday I've found an essential problem in unit test for Resolver (/spec/unit/resolver_spec.js:36) - it assumes that /A and /B are final absolute paths, which is not true on Windows (drive letter should be specified, like C:\A), which leads to test fail, because call to path.resolve in /lib/sources/local.js:10 returns the path with drive letter prepended (it's C: by default), and "/C.path.B" != "C:\C.path.B" .

That's the reason of failure for the first five unit tests of Resolver.

Another problem similar to the one I've reported today ( https://github.com/sdarnell/meteor/issues/27 ): smart lock unit test (/spec/unit/smart_lock_spec.js:126) assumes that forward slashes are used to separate directories, which is not true on Windows (backslashes are used here). Therefore, paths should be like ..\\..\\mrt-test-pkg1. I think that path.join should be used here.

These six are the only tests that fail on my system before mrt add tests.

@yeputons
Copy link
Contributor Author

@tmeasday One more problem: you call process.exit() in /bin/mrt.js, but it does not flush output buffers. Sometimes this leads to mrt search not returning correct JSON and assertIncreasesInstallCount fails.

This particular issue is not Windows-related, I will open a separate ticket and, after some time, pull request (I already know how to fix the problem in one particular case).

@yeputons
Copy link
Contributor Author

Windows-specific issue on stdout/stderr flushing after exit: nodejs/node-v0.x-archive#3584 . It's already two years old.

UPD: workaround package

@seanmwalker
Copy link

@yeputons, thanks for the list of points, I’m on board with you. Here’s a quick response on them, I think we’re largely ready to go. I’m going to start a little tonight, and should be on it each evening this week. Let me know how you’d like to carve up the work, I was going to try to get the directory creation (2, 3 and 4) and paths addressed first. Did you want to continue with 1?

  1. If we can get things working as you have started with the spawn-cmd, that’d be great. I saw your note about the default js extension launching in the jscript process instead of node. Let me know if you run into a permanent roadblock and want a second set of eyes on it, otherwise I’ll assume we’re going with this approach.
  2. The mklink for junction is at the OS level, it’s been there since vista so I think we’re in good shape using this across Windows versions. I’ve not had any issues unlinking,
  3. I was considering that or another library, but I think we’ll still want a wrapper function to pop off the trailing directory so git can create that one.
  4. I have not experienced too many issues with unlink in my work, but admittedly I have not had to use it a lot. I’ve usually found the issue was that something else had the file open (an editor, or even windows explorer showing the directory). I see a number of posts about unlinking issues, many result in this sort of issue, but a few revolve around the size of the file. Windows had the hardware abstraction layer which can create some delays in processing, so we may just need to be careful about how we handle unlinking. Another note, it needs to be a full path for Windows to work correctly as well.
  5. I like meteorite as well for the additional name.
  6. Thanks @tmeasday, great solution branching this. I'm looking forward to this.

@seanmwalker
Copy link

Just a note, I've created the branch called 'windows-updates' in meteorite.

@sdarnell
Copy link

Just a couple of notes from my experiences with Meteor on Windows.
Windows is just a pain with respect to symlinks. It has a number of variants with file and directory symlinks being slightly different. A key issue is that unless you change the local security policy, or run as an elevated admin I think the only symlink you can create is a directory junction. Windows Node also has bugs with symlinks.
This isn't too bad, but many applications just don't understand junctions sufficiently and can cause havoc.
In the end I decided to avoid symlinks entirely, and luckily for the main Meteor there are only a couple of places and in those I do manual redirection with marker files.

Unlink is also a pain on Windows. On unix-based platforms you can always unlink files and they disappear from the filesystem even if the file is opened by someone. On Windows you cannot delete files that are in use. Grrr!
Some apps will only release files after an attempt has already been made to delete it. There are also some other scenarios possibly due to antivirus which often means that to reliably delete a directory you need to retry with delays etc. (Double Grrr).

@DoubleU23
Copy link

✅ Got rid of the unlink errors (EPERM) per installing graceful-fs globally.
It's a little bit slower but it seems to work. Maybe some of you wanna test it.

Would like to hear some feedback.

@sdarnell
Copy link

Windows meteor does something similar to graceful-fs, but instead of busy looping until it completes it uses a fibers based sleep() to allow AV to get out of the way.

@yeputons
Copy link
Contributor Author

So far two more things to fix:

5.process.env.HOME does not exist on Windows. At least, it's not specified by default. I think process.env.HOME || (process.env.HOMEDRIVE + process.env.HOMEPATH) is the best option.
6. killProcessFamily function in /spec/lib/runner.js uses ps to get list of all children processes and kill them. It's used during tests, when Meteor is started and should be killed right after it reports success

@tmeasday
Copy link
Member

@yeputons - re 6. - we need to do this to make sure that the proxied meteor server and the mongo server are killed when we kill the meteor process. If someone has a better way of doing this, I'd be very glad to get rid of this hack.

@yeputons
Copy link
Contributor Author

One more thing:

7.Meteor warehouse is

    if (process.platform === 'win32') {
      var home = process.env.LOCALAPPDATA || process.env.APPDATA;
      return path.join(home, '.meteor');
    }

    return path.join(process.env.HOME, '.meteor');

So it's different on Windows. It's used in /spec/acceptance/test_spec.js.

UPD: Looks like it can be deleted - created an issue for that: #243
UPD2: No, it's required on Linux and should be fixed on Windows.

yeputons added a commit that referenced this pull request Jan 20, 2014
…s now it's changed iff there was no such env variable before
yeputons added a commit that referenced this pull request Jan 20, 2014
yeputons added a commit that referenced this pull request Jan 20, 2014
yeputons added a commit that referenced this pull request Feb 3, 2014
…ich waits till stdout/stderr buffers are flushed on Windows before exit
@yeputons
Copy link
Contributor Author

yeputons commented Feb 3, 2014

Please note that I've added the exit NPM package because of pipes buffering issues ( nodejs/node-v0.x-archive#3584 ) during tests.

yeputons added a commit that referenced this pull request Feb 3, 2014
@yeputons
Copy link
Contributor Author

yeputons commented Feb 3, 2014

By now windows-updates branch passes all tests on Linux (sometimes one of the last tests about Meteor pinning fails because 15 seconds is not enough for my slow internet to download the bunch) and all but three tests on Windows (the ones about pinning Meteor, because this pinned version does not support Windows and therefore are not starting).

We have only two things left in TODO: use spawn-cmd instead of spawn('a.bat') and try to find some unlink problems :)

@seanmwalker
Copy link

One thing I noticed this weekend was that if there is a space in a directory name (in my case both my test application, meteorite and atmosphere are in a directory called 'Source Code' ), the process throws an exception. I haven't had a chance to dig into it, but thought we may want to add that to the list.

@yeputons
Copy link
Contributor Author

yeputons commented Feb 4, 2014

@seanmwalker I had this issue too. Not sure if it's fixed now, but I've added some quotes to exec's first argument. Unfortunatelly, I haven't tested it yet.

@yeputons
Copy link
Contributor Author

yeputons commented Feb 4, 2014

@tmeasday Would you mind describing the exact purpose of these lines? I don't get it. For example, what if METEOR_WAREHOUSE_DIR was already specified by a user (for example, he don't like ~/.meteor and wants to use ~/.soft/meteor instead)?

yeputons added a commit that referenced this pull request Feb 4, 2014
@tmeasday
Copy link
Member

tmeasday commented Feb 4, 2014

@yeputons - I don't remember very well but I think the reason for this was to make sure that Meteor doesn't have to re download itself for every test (as we destroy the home directory each time).

If METEOR_WAREHOUSE_DIR isn't set, it uses ~/.meteor, so we set it to that explicitly before changing the home directory.

Probably the line should be:

if (! process.env.METEOR_WAREHOUSE_DIR)
  process.env.METEOR_WAREHOUSE_DIR = path.join(process.env.HOME, '.meteor')

@yeputons
Copy link
Contributor Author

yeputons commented Feb 8, 2014

One more thing in TODO: I've noticed that meteorite uses unix newlines in some files that are changed and it makes my git client very sad.

@yeputons
Copy link
Contributor Author

spawn-cmd was added. Here's what happens with tests:

  1. Linux, no spaces in folder name: all passed (126)
  2. Linux, folder with spaces: all except should run the forked meteor passed (123).
  3. Windows, no spaces in folder name: all except should run the forked meteor passed (123).
  4. Windows, folder with spaces: a lot of test failed because of If both executable and at least one argument have spaces, nothing works featurist/spawn-cmd#3 (UPD: in fact, this issue is even more serious - if both path to git and one of its arguments have space, the whole thing fail).

@yeputons
Copy link
Contributor Author

@tmeasday @seanmwalker What do you guys think about the following:

  1. Line endings in smart.json/smart.lock.
  2. Unlinking issues. I haven't noticed any, do you?
  3. Custom meteor-related tests.

?

@seanmwalker
Copy link

I've been pretty happy with how things are working for my 3 machines, 2 windows and 1 linux. I do have the same git setup on both windows machines, so I should try changing that to see if there are any issues (different options for command line access to utilities, line endings for files etc). I will recheck things using the latest change for spawn command though.

  1. Git lets windows users specify the types of line endings when installing git. I wonder if that git install option the cause of some of the issues? Maybe we can read the git config on windows and use whatever is configured there on the smart.json file?

  2. I haven't been able to find any unlinking issues at this time. So I'm reasonably comfortable it's going to not too worried about it with this app at this time. The biggest issue would be any callback that holds a file open, or someone having a directory open on the machine.

  3. Did you have some areas in mind for testing?

@yeputons
Copy link
Contributor Author

@seanmwalker

  1. That will be cool.
  2. I was talking about three last tests that are failing because Meteorite's test Meteor does not work on Windows (and it not easy to fix as I know). What should we do with them? I'm thinking about ignoring them and documenting it/explicitly disabling this feature on Windows.

@tmeasday
Copy link
Member

@yeputons is the issue with spaces under linux a problem that was there before or was it introduced by using the alternate spawn-cmd library?

@yeputons
Copy link
Contributor Author

@tmeasday it was even worse before - it failed at 'before all' hook. But if I add extra quotes to baseCommand at /spec/lib/atmosphere.js, we have 123 passed tests and 3 failed (the last ones, same as on windows-updates branch)

@tmeasday
Copy link
Member

Oh, sure. But in practice did it work to run custom Meteor out of a directory with spaces? (I guess I can test this easily enough...)

@yeputons
Copy link
Contributor Author

@tmeasday not sure how to test it. Doesn't README.md says that the mrt command and pinning meteor version in smart.json is deprecated? Anyway, this won't work cross-platformly on Windows. We may, however, add special test apps which are pinned to Windows branch of Meteor. I'll try this.

@yeputons
Copy link
Contributor Author

@tmeasday Well, Meteor cannot be easily runned from checkout on Windows - it wants to use dev-bundle. I think that the best option is explicitly disable pinning Meteor on Windows (but add a special key like --force for those who really knows what they do) and print warnings if it's specified in smart.lock/smart.json.

@tmeasday
Copy link
Member

Ok, sounds reasonable

@tmeasday
Copy link
Member

@yeputons - for the record running mrt with a forked Meteor does currently work in a directory with spaces.

I have seen issues before where people use spaces in e.g. their home directory. So ideally we wouldn't regress here. But it's not the greatest priority I guess.

I do say it's deprecated, but at the same time I do it in one of my own projects! I think maybe deprecated is the wrong word. I really just didn't want people doing it when it wasn't necessary as a lot of Meteor issues were getting opened against Meteorite...

@yeputons
Copy link
Contributor Author

@tmeasday would you mind giving an example smart.json or example project (latter is better) so I can test it?

@yeputons
Copy link
Contributor Author

@tmeasday I took app-with-meteor-pinned-to-branch as an example and both master and windows-updates branches are able to download and correct version of Meteor on Linux. I think that the problem with npm test is that path to home folder (where .meteorite lies) have spaces during testing, which is very unusual for Linux.

@yeputons
Copy link
Contributor Author

@tmeasday It looks like Meteor itself doesn't know how to install from folders with spaces.

UPD: however, if I manually download last version of Meteor and run it from checkout (all paths have spaces), everything works.

@yeputons
Copy link
Contributor Author

@tmeasday Yes, it's old version of possibilities/meteor that is unable to start if $0 contains spaces, it's fixed in main Meteor repository. No regression so far, would you mind fixing this meteor version and updating apps?

@tmeasday
Copy link
Member

@yeputons - I tried (update-meteor-version) but the tests kept failing / timing out.

I think because newer versions of Meteor take bloody ages to start on a new project (downloading npm packages, initialising mongo etc). This means using such a Meteor version in our tests is going to massively blow out the time it takes to run the tests or we are going to figure out some way around the problem.

I'm not really sure what the best thing to do is.

I'm not too stressed about the tests not passing in a directory with spaces, as long as Meteorite itself still works ok. It's a strange use case at the best of times. So I'd probably vote to just leave it.

@tmeasday tmeasday closed this Feb 17, 2014
@tmeasday tmeasday reopened this Feb 17, 2014
@dotnetwise
Copy link

So 9 months later this is still dead in its tracks?

@yeputons
Copy link
Contributor Author

yeputons commented Oct 2, 2014

@dotnetwise Looks so :( However, I've been using this branch of Meteorite on Windows for a several months and didn't notice any annoying problems (there can be some hidden, of course). It's very easy to install: just clone repo, switch to this branch and call npm link meteorite.git.

Btw, Meteor 0.9 came out, and Meteorite will be no longer be used after some time as there is built-in package manager now.

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.

7 participants