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 updates: seconds attempt #248

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

Windows updates: seconds attempt #248

wants to merge 38 commits into from

Conversation

yeputons
Copy link
Contributor

@tmeasday @seanmwalker @sdarnell I think that it's time to create a new pull request so that we can see all changes in one place. I've disabled meteor pinning for Windows and added some line endings heuristics which tries to preserve type of EOLs in smart.lock, smart.json and .gitignore (os.EOL is used by default). meteorite create-project creates stub project with system-specific EOLs.

No spaces folder:

  1. All tests are passed on Linux (126)
  2. All tests are passed on Windows (123, yellow warning is displayed before tests)

Folder with spaces (it implies that HOME directory have spaces during testing too):

  1. Three meteor-pinning tests are failing on Linux because of old Meteor testing versions (these versions themselves are not able to correctly run from folder with spaces).
  2. Lots of tests are failing on Windows because of If both executable and at least one argument have spaces, nothing works featurist/spawn-cmd#3 (spaces issue). However, creating and working with projects which have spaces in path is OK and works fine, because we run git by its name during regular work, not by its absolute path and therefore this path is not getting quoted.

Things left to do:

Waiting for your comments, code review and thoughts on the second TODO thing.

seanmwalker and others added 30 commits January 20, 2014 01:47
…EPATH, and added in the wich package for fining git, curl and meteor.

This seems to work reasonably well in cmd.exe and in cygwin.

Still working on test issues though.
…s now it's changed iff there was no such env variable before
…s via a shared function for building the full path based on the subdirectory path provided. Updated the tests to call meteorite instead of mrt, so that windows will be able to execute them as well. There are a lot of tests to address to get the windows execution working at 100%.
…ve() to be consistent with what package.js do on Windows (driver letter issue)
Conflicts:
	spec/acceptance/test_spec.js
… windows consistent in both cygwin and cmd.

Changed the mrt.js call to use the meteorite.cmd file created by 'npm install'. This fixed 38 tests for me.
 - TODO: Refactor how we get the paths for tests, fix the 3 remaining tests and clean up a few odds and ends.
Conflicts:
	spec/lib/atmosphere.js
	spec/support/bin/curl
…ectory is pre-created as a workaround for windows git
…s - it's one of ./bin/mrt.{js,bat} (the latter was added)
…oriteExecutable to work OK from folders with spaces
…ich waits till stdout/stderr buffers are flushed on Windows before exit
…ch added '.bat' to 'meteor' command before spawn-cmd
@Spiralis
Copy link

As for all I can say this looks good. Does seem like the second issue might be resolved (featurist/spawn-cmd#3), but the issue is still open it seems.

@Hades32
Copy link

Hades32 commented May 23, 2014

The usage of junctions makes it mandatory to run mrt as administrator. This is not very nice and should at least be mentioned in the readme.

"Ordinary" symbolic links can AFAIK be created by normal users, but that has also drawbacks. I think there are some libraries that fake unix style symbolic links, maybe that would be a better approach?

@yeputons
Copy link
Contributor Author

@Hades32 are you sure about directory junctions? It's not the same as symbolic/hard link on Windows. I've just tried to create one under a restricted user and it succeeded, while creation of hard link to file/directory failed because of lack of rights.

Moreover, this branch of Meteorite works just fine under restricted user too.

@seanmwalker
Copy link

@Hades32 were you using a directory path outside of your users directory?
(A folder outside of C:\users{your username} for instance. You can find your user directory with the USERPROFILE environment variable if you're not sure)

@Hades32
Copy link

Hades32 commented May 29, 2014

Well, I tested it that way:

C:\Temp\syn>mklink /D SymbolicDirectory RealDirectory
Ihre Berechtigungen reichen nicht aus, um diesen Vorgang auszuführen. (NO PERMISSION)

C:\Temp\syn>mklink /J JunctionDirectory RealDirectory
Verbindung erstellt für JunctionDirectory <<===>> RealDirectory (SUCCESS)

C:\Temp\syn>mklink /H HardDirectory RealDirectory
Zugriff verweigert (ACCESS DENIED)

So, yes, I'm testing outside of my user directory, but I'm the owner of the directory and I'm administrator (just running an ordinary (not elevated) prompt.

@yeputons
Copy link
Contributor Author

@Hades32 so what's the problem? Meteorite use the second way (directory junctions) and your log says it's OK, isn't it?

@Hades32
Copy link

Hades32 commented May 30, 2014

Well, maybe I made some false assumptions there, but my problem essentially is this:

C:\Temp\fahrtenbuch>meteorite

Stand back while Meteorite does its thing
V router
    tag: https://github.com/tmeasday/meteor-router.git#v0.6.1

fs.js:730
  return binding.symlink(preprocessSymlinkDestination(destination, type),
                 ^
Error: EPERM, operation not permitted 'C:\Users\Martin\AppData\Local\.meteorite\packages\router\tmeasday\meteor-router\8ec75fec7affdefc787d19f23b36fd6d1d44ef04'

    at Object.fs.symlinkSync (fs.js:730:18)
    at C:\OtherProgs\meteorite\lib\dependencies\package.js:131:10
    at C:\OtherProgs\meteorite\lib\sources\git.js:84:15
    at GitSource._load (C:\OtherProgs\meteorite\lib\sources\git.js:142:12)
    at C:\OtherProgs\meteorite\lib\sources\git.js:82:18
    at C:\OtherProgs\meteorite\lib\sources\git.js:98:5
    at C:\OtherProgs\meteorite\lib\sources\git.js:264:5
    at ChildProcess.exithandler (child_process.js:645:7)
    at ChildProcess.EventEmitter.emit (events.js:117:20)
    at maybeClose (child_process.js:753:16)

@yeputons
Copy link
Contributor Author

yeputons commented Jun 2, 2014

@Hades32 are you able to reproduce this issue in another directory? Say, clean check out of some project in your home directory? Are you able to manually create directory junction between the path Meteorite displayed and corresponding directory in packages/?

@Hades32
Copy link

Hades32 commented Jun 9, 2014

Interestingly I cannot reproduce inside my user directory. As both are owned by the same user I don't really understand why though. I AM able to create a junction between above path and the packages dir - so that doesn't really give a hint either...
But at least I can now comfortably work when I use my user directory - thanks for that!

@rudolfb
Copy link

rudolfb commented Jul 22, 2014

@yeputons
Just wanted to thank you for your work in getting mrt working under Windows.

Managed to get your branch working by doing the following on my Windows 8 workstation:

Cloned the oortcloud/meteorite repo
Switched to your branch: https://github.com/oortcloud/meteorite/tree/windows-updates

Needed to install the prerequisite npm packages, and then meteorite:

$ npm install -g ddp,underscore,wrench,fstream,optimist,prompt,colors,async,rolling_timeout_exec,which,exit,spawn-cmd,mocha,connect,meteorite
$ npm install -g meteorite

Had to change some of the package version numbers in the package.json:

  • , "underscore": "1.3.3"
  • , "underscore": "1.6.0"
  • , "prompt": "0.2.11"
  • , "colors": "0.6.0-1"
  • , "async": "0.2.9"
  • , "prompt": "0.2.13"
  • , "colors": "0.6.2"
  • , "async": "0.9.0"

After this, I got your code working, as long as theC:\Users\[User]\AppData\Roaming\npm is on one's path.

meteorite did not work for me, but if I use mrt.cmd, e.g. mrt.cmd add luma-ui or simply mrt.cmd, then the commands all seem to work.

Sometimes after issuing a mrt.cmd update it seemed as if the process got hung up. Pressed CTRL-C, then reissued the command and everything was fine. But I had this happen using a meteor update as well, so I doubt this has something to do with your modifications.

All in all it seems to work. Tried these steps on a second PC successfully as well. As long as the C:\Users\rb\AppData\Roaming\npm is on my path, then calling mrt.cmd works fine for me.

Again, thanks.

Regards

Rudolf

Windows PowerShell
Copyright (C) 2013 Microsoft Corporation. Alle Rechte vorbehalten.

PS C:\Users\rb> npm install -g ddp,underscore,wrench,fstream,optimist,prompt,colors,async,rolling_timeout_exec,which,exi
t,spawn-cmd,mocha,connect
C:\Users\rb\AppData\Roaming\npm\which -> C:\Users\rb\AppData\Roaming\npm\node_modules\which\bin\which
C:\Users\rb\AppData\Roaming\npm\_mocha -> C:\Users\rb\AppData\Roaming\npm\node_modules\mocha\bin\_mocha
C:\Users\rb\AppData\Roaming\npm\mocha -> C:\Users\rb\AppData\Roaming\npm\node_modules\mocha\bin\mocha
[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\rolling_timeout_exec

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\which

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\spawn-cmd

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\colors

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\underscore

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\async

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\wrench

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\exit

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\fstream
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\optimist
+-- [email protected]
+-- [email protected]

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\ddp
+-- [email protected]
+-- [email protected]
+-- [email protected] ([email protected])

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\connect
+-- [email protected]
+-- [email protected]
+-- [email protected] ([email protected])
+-- [email protected] ([email protected], [email protected])

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\mocha
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected] ([email protected])
+-- [email protected] ([email protected], [email protected])
+-- [email protected] ([email protected], [email protected], [email protected])

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\prompt
+-- [email protected]
+-- [email protected]
+-- [email protected] ([email protected])
+-- [email protected] ([email protected], [email protected], [email protected], [email protected], [email protected], [email protected])
+-- [email protected] ([email protected], [email protected], [email protected], [email protected], [email protected], [email protected])
PS C:\Users\rb> npm install -g meteorite
C:\Users\rb\AppData\Roaming\npm\mrt -> C:\Users\rb\AppData\Roaming\npm\node_modules\meteorite\bin\mrt.js



> [email protected] postinstall C:\Users\rb\AppData\Roaming\npm\node_modules\meteorite
> sh ./completions/postinstall.sh

[email protected] C:\Users\rb\AppData\Roaming\npm\node_modules\meteorite
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected] ([email protected], [email protected])
+-- [email protected] ([email protected], [email protected], [email protected], [email protected])
+-- [email protected] ([email protected], [email protected], [email protected])
+-- [email protected] ([email protected], [email protected], [email protected], [email protected], [email protected])
PS C:\Users\rb> 

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.

5 participants