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

braindump for windows #1

Merged
merged 1 commit into from
Nov 28, 2014
Merged

Conversation

stefanpenner
Copy link
Contributor

this isn't meant as a solution, but as a place to experiment with window support (as time permits)

symlinks apparently work if:

  • the user is an admin (confirm)
  • the users policy allows them (couldn't get this to work)
  • get feedback from @joliss

with more hacks i was able to get ^^ sorta booting, but I ran into other problems (like missing files) and I failed to have time to investigate further today

fs: fs
};

var spawn = require('child_process').spawn;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odd indentation here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this isn't used, remove?

@joliss
Copy link
Member

joliss commented Oct 5, 2014

What happens if we always hardlink on win32?

I'm kinda bracing myself for another episode in disastrous hardlink handling like OS X, but on the other hand symlinks are not much fun either, so perhaps it's worth exploring.

return false;
}

fs.unlinkSync(canLinkDest);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need to unlink canLinkSrc here. Otherwise it is only removed if an error occurs during symlinking.

@rwjblue
Copy link
Member

rwjblue commented Oct 5, 2014

@joliss - I was thinking that we could do a tiered strategy on windows: symlink (if we can), hardlink (if we can), then fall back to copyDereferenceSync. The hardlinking logic will essentially be what I have been using in broccoli-caching-writer (from here).

Ray, Stef, and I will be working through this in the next few days....

@joliss
Copy link
Member

joliss commented Oct 6, 2014

Yes, maybe a tiered strategy is good. (I also just remembered that we probably cannot hardlink across device boundaries, so we'd still need a copy fallback.)

@BryanCrotaz
Copy link

Seems like junctions are what we need - power of hardlinks without the restrictions?

@stefanpenner
Copy link
Contributor Author

@Twinkletoes one of my early spikes used junctions for directories which worked great. Unfortunately we also need something for individual files currently. So maybe junctions if available for directories, the copy/symlink/hardlink as possible for files that need to be brought in

@BryanCrotaz
Copy link

individual files can be junctioned (is that a word?)
http://msdn.microsoft.com/en-gb/library/windows/desktop/aa365503(v=vs.85).aspx

@stefanpenner
Copy link
Contributor Author

@Twinkletoes interesting, for some reason this was failing for me..

@stefanpenner
Copy link
Contributor Author

@Twinkletoes i've added an issue for a test suite, this way we can iterate much quicker. #3

@pgasiorowski
Copy link

Just noticed that there is a need for improving windows experience in broccoli and ember and thought I could provide some help with testing this library.

Here are results for running tests from @stefanpenner PR:

  node-symlink-or-copy
    V windows falls back to copy
    V windows symlinks when has permission

  testing mode
    V allows fs to be mocked

  3 passing (7ms)

Although running the example code from README.md:

var symlinkOrCopySync = require('symlink-or-copy').sync;

symlinkOrCopySync('src_dir/some_file.txt', 'dest_dir/some_file.txt');
symlinkOrCopySync('src_dir/some_dir', 'dest_dir/some_dir');

works for the first time then (2nd attempt) it gives similar error message I posted in #2. Is this intentional that destination folder is not getting cleaned before copying/linking? I am sorry if I'm getting something wrong here. Here is the fill stack trace:

Error: EEXIST, file already exists 'F:\SVN\node-symlink-or-copy\dest_dir\some_file.txt'
    at Object.fs.openSync (fs.js:427:18)
    at Object.fs.writeFileSync (fs.js:966:15)
    at Object.copyDereferenceSync (F:\SVN\node-symlink-or-copy\node_modules\symlink-or-copy\node_modules\copy-dereference\index.js:26:8)
    at symlinkOrCopySync (F:\SVN\node-symlink-or-copy\node_modules\symlink-or-copy\index.js:61:13)
    at Object.<anonymous> (F:\SVN\node-symlink-or-copy\index.js:3:1)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)

@ccoenen
Copy link

ccoenen commented Oct 8, 2014

i seem to remember that junctions and/or softlinks and/or hardlinks are limited to users with administrator-privileges. Sadly, I can't find any documentation on that.

@inDream
Copy link

inDream commented Oct 15, 2014

I've already tried implement junction for symlink-or-copy,
it works for creating junction but time only reduced by 10 sec.
https://github.com/inDream/node-symlink-or-copy/tree/windows-spike

Before

Build successful - 51450ms.

Slowest Trees                  | Total
-------------------------------+----------------
CustomStaticCompiler           | 22233ms
TreeMerger (stylesAndVendor)   | 8161ms
TreeMerger (appAndDependencies) | 7902ms
TreeMerger (ExternalTree)      | 7650ms

After

Build successful - 39566ms.

Slowest Trees                  | Total
-------------------------------+----------------
TreeMerger (appAndDependencies) | 13330ms
TreeMerger (stylesAndVendor)   | 11461ms
TreeMerger (ExternalTree)      | 9804ms

@stefanpenner
Copy link
Contributor Author

@inDream can you provide more information, eg. what sort of IO is going on during the build phase? Do you have SSD or HD, what sort of machine, what OS what node etc.

We should likely script a plausible broccoli run, and compare various approaches and try and isolate the bottle necks.

@inDream
Copy link

inDream commented Oct 15, 2014

Log files with ProcessMonitor:
csv: https://app.box.com/s/4bbnhpz0bdu8hk6x70bq
pml: https://app.box.com/s/o17t18g226qo3cn8alti

@stefanpenner
Copy link
Contributor Author

@inDream thank you kindly

@gblmarquez
Copy link

@inDream I'm working on the same thing https://github.com/gblmarquez/node-symlink-or-copy/tree/windows-spike

Maybe we could work together.

@BryanCrotaz
Copy link

I'm looking to help with this too but going through a major client rollout
until mid nov. Can help after that.

Bryan Crotaz
Silver Curve

On 15 Oct 2014, at 21:59, Gabriel Marquez notifications@github.com wrote:

@inDream https://github.com/inDream I'm working on the same thing
https://github.com/gblmarquez/node-symlink-or-copy/tree/windows-spike

Maybe we could work together.


Reply to this email directly or view it on GitHub
#1 (comment)
.

gblmarquez referenced this pull request in gblmarquez/node-symlink-or-copy Oct 15, 2014
@jonnii
Copy link

jonnii commented Oct 16, 2014

Is there a way to get ember-cli to use this branch as the dependency for node-symlink-or-copy? I can test this out on my machine at work.

@inDream
Copy link

inDream commented Oct 16, 2014

I've changed broccoli-merge-trees & reduced the build time from 51450ms to 3939ms.
https://github.com/inDream/broccoli-merge-trees

But I don't know I am doing right or not. This line caused error in windows.

Build successful - 3939ms.

Slowest Trees                  | Total
-------------------------------+----------------
SassCompiler                   | 1251ms
ES3SafeFilter                  | 494ms
ES6Concatenator                | 332ms
JSHint - App                   | 262ms
TreeMerger (appAndDependencies) | 224ms
JSHint - Tests                 | 201ms

@inDream
Copy link

inDream commented Oct 16, 2014

@jonnii You can download the zip content from GitHub and replace the original folder.

@stefanpenner
Copy link
Contributor Author

@inDream ah, im surprised mergeTree's doesn't already rely on symlink or copy...

@stefanpenner
Copy link
Contributor Author

@inDream you are going to bring the dreams of the windows ember-cli community to reality!

@stefanpenner
Copy link
Contributor Author

But I don't know I am doing right or not. This line caused error in windows.

I'm pretty dubious of process.cwd() usages period, it is almost never the right thing.

@diogomafra
Copy link

I tested my project using this branch and the performance is much better.

To test it I just copyed the files in this brach to every "symlink-or-copy" folder inside my project.


================= BEFORE ========================

Build successful - 20380ms.                              

Slowest Trees                  | Total                   
-------------------------------+----------------         
CustomStaticCompiler           | 4191ms                  
TreeMerger (ExternalTree)      | 2216ms                  
CustomStaticCompiler           | 2038ms                  
EsnextFilter                   | 1858ms                  
TreeMerger (appAndDependencies) | 1844ms                 
TreeMerger (stylesAndVendor)   | 1699ms                  
ES3SafeFilter                  | 1609ms                  
TreeMerger                     | 1050ms                  

file changed pods\application\route.js                   

Build successful - 9322ms.                               

Slowest Trees                  | Total                   
-------------------------------+----------------         
TreeMerger (stylesAndVendor)   | 1717ms                  
TreeMerger                     | 1037ms                  
TreeMerger (appAndDependencies) | 973ms                  
TreeMerger (ExternalTree)      | 954ms                   
CustomStaticCompiler           | 566ms                   
TreeMerger (vendor)            | 496ms                   


================= AFTER ========================

Build successful - 4398ms.

Slowest Trees                  | Total
-------------------------------+----------------
EsnextFilter                   | 1812ms
ES3SafeFilter                  | 808ms
JSHint - App                   | 502ms

file changed pods\application\route.js

Build successful - 1694ms.

Slowest Trees                  | Total
-------------------------------+----------------
ES3SafeFilter                  | 403ms
EsnextFilter                   | 185ms
TreeMerger (appAndDependencies) | 137ms

@raytiley
Copy link
Contributor

Doc PR for Ember CLI here: ember-cli/ember-cli#2563

@BryanCrotaz
Copy link

We're back into writing Ember stuff after an enforced break. What's the
best way to try out the new broccoli branch?

Bryan

On 26 November 2014 at 00:47, Ray Tiley notifications@github.com wrote:

Doc PR for Ember CLI here: ember-cli/ember-cli#2563
ember-cli/ember-cli#2563


Reply to this email directly or view it on GitHub
#1 (comment)
.

Bryan Crotaz
Managing Director
Silver Curve

@karl-sjogren
Copy link

I tried as @diogomafra said and the results are fantastic!

Windows 8.1 running in Parallels on a Macbook Pro Early 2013 with an ssd drive.

================= BEFORE ========================

Build successful - 50745ms.

Slowest Trees                  | Total
-------------------------------+----------------
TreeMerger (appAndDependencies) | 7512ms
TreeMerger (stylesAndVendor)   | 6309ms
TreeMerger (ExternalTree)      | 6234ms
CustomStaticCompiler           | 5940ms
JSHint - App                   | 2638ms
ES3SafeFilter                  | 2594ms

file changed router.js

Build successful - 43748ms.

Slowest Trees                  | Total
-------------------------------+----------------
TreeMerger (appAndDependencies) | 9213ms
TreeMerger (stylesAndVendor)   | 7095ms
TreeMerger (ExternalTree)      | 6691ms
CustomStaticCompiler           | 4543ms



================= AFTER ========================

Build successful - 14160ms.

Slowest Trees                  | Total
-------------------------------+----------------
ES3SafeFilter                  | 2726ms
JSHint - App                   | 2336ms
AutoprefixerFilter             | 1797ms
LessCompiler                   | 1772ms
ES6Concatenator                | 1289ms
TemplateCompiler               | 812ms

file changed router.js

Build successful - 5710ms.

Slowest Trees                  | Total
-------------------------------+----------------
Funnel                         | 1044ms
ES3SafeFilter                  | 736ms
AutoprefixerFilter             | 647ms
JSHint - App                   | 628ms
TemplateCompiler               | 346ms
StaticCompiler                 | 331ms

This is from a fairly large project with a lot of external dependencies (bower_components is over 20mb and our dist-folder on a debug build is 13mb) and we also have some custom broccoli-filters that seem to do just fine when testing this.

@karl-sjogren
Copy link

And this is another test done on one of our support guys computers with the same app.

Windows 7 on a Dell laptop, don't know the specs. No ssd drive.

================= BEFORE ========================

Build successful - 106919ms.

Slowest Trees                  | Total
-------------------------------+----------------
TreeMerger (appAndDependencies) | 21267ms
TreeMerger (ExternalTree)      | 14910ms
CustomStaticCompiler           | 13670ms
Funnel                         | 12932ms
TreeMerger (stylesAndVendor)   | 7136ms

file changed router.js

Build successful - 60417ms.

Slowest Trees                  | Total
-------------------------------+----------------
TreeMerger (stylesAndVendor)   | 10915ms
TreeMerger (ExternalTree)      | 7072ms
TreeMerger (appAndDependencies) | 6164ms
ES3SafeFilter                  | 5208ms
CustomStaticCompiler           | 5001ms


================= AFTER ========================

Build successful - 10851ms.

Slowest Trees                  | Total
-------------------------------+----------------
ES3SafeFilter                  | 3226ms
JSHint - App                   | 1569ms
LessCompiler                   | 1359ms
AutoprefixerFilter             | 1007ms
ES6Concatenator                | 949ms

file changed router.js

Build successful - 6500ms.

Slowest Trees                  | Total
-------------------------------+----------------
StaticCompiler                 | 1226ms
ES3SafeFilter                  | 800ms
JSHint - App                   | 657ms
TreeMerger                     | 644ms
Funnel                         | 587ms
AutoprefixerFilter             | 427ms

@stefanpenner
Copy link
Contributor Author

many windows people are going to be excited :)

Also for reference thanks to one of our clients, I'll be doing 3 full windows + ember-cli days next week.

Hopefully @joliss has a chance for feedback before then.

@tsing80
Copy link

tsing80 commented Nov 27, 2014

Except Windows XP user :-(. I will continue to use a gulpjs file until my company have migrate to Windows 7,which may also not have administrator privileage

@stefanpenner
Copy link
Contributor Author

@tsing80 well, we welcome more contributions to help improve the lives of windows XP users. Do you have any ideas or suggestions?

Unfortunately even Microsoft has dropped support for Windows XP: http://windows.microsoft.com/en-us/windows/end-support-help our poor little team of OSS volunteers really doesn't have the bandwidth to support it unless someone champions it.

}

function testCanSymlink () {
var canLinkSrc = path.join(__dirname, "canLinkSrc.tmp")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole thing seems super-brittle... I think we should just return true if !isWindows, so at least we don't incur the brittleness on Mac/Linux.

I suspect __dirname might not always be writable, e.g. if the currently executing script was installed in a directory only writable by adminstrator. Is there a directory that is reliably writable? Would cwd generally work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the global tmp dir is the only place we can safely do this.

@joliss
Copy link
Member

joliss commented Nov 27, 2014

So I'm thinking, the code to deal with the Unix/Windows differences is starting to look more and more gnarly, with if isWindows in several places. I wonder if we should just split the whole module in half, with one big if isWindows at the top, providing one complete implementation for Windows and one for Unix? It might be simpler and easier to understand & maintain.

@raytiley
Copy link
Contributor

@joliss how does this look now?

var canLinkDest = path.join(tmpdir, "canLinkDest.tmp")

try {
fs.writeFileSync(canLinkSrc);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This writes undefined into the file. Perhaps pass an empty string as second argument to future-proof it?

Other than that it looks good to me.

@raytiley and @stefanpenner, are you going to be around for maintaining the Windows parts on this? I can't maintain it myself because I don't have a Windows machine around (one that's set up for development anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we will maintain it.

Is it possible for us to get commit + release to NPM ability. We can be sure to swing big changes by you.

@raytiley
Copy link
Contributor

@joliss @stefanpenner
Writing empty string like suggested...

I'm not going anywhere.

@joliss
Copy link
Member

joliss commented Nov 27, 2014

Sweet. Do you want to squash the commits, and then do you want me to merge and release v1.0.1?

@rwjblue
Copy link
Member

rwjblue commented Nov 27, 2014

@joliss - Yes, please.

@raytiley
Copy link
Contributor

I squashed this into one commit. @stefanpenner and @duizendnegen should also get credit, but I have no idea how to do that.

@stefanpenner
Copy link
Contributor Author

just add @ prefixed names to the bottom of the commit. Ultimately you are pushing it over the finish line you are welcome to the primary credit :)

@raytiley
Copy link
Contributor

Ship It!

Ship It

joliss added a commit that referenced this pull request Nov 28, 2014
@joliss joliss merged commit 11dbf1a into broccolijs:master Nov 28, 2014
@joliss
Copy link
Member

joliss commented Nov 28, 2014

Lovely, thanks everyone. It's out in v1.0.1.

Happy Thanksgiving!

@jonnii
Copy link

jonnii commented Nov 28, 2014

It's a Thanksgiving Miracle!! 🎉 🎈 🎉 🎈

@BryanCrotaz
Copy link

Following the instructions at
https://github.com/stefanpenner/ember-cli/releases...

Unknown version 1.0.1?

G:\Projects\package-ui>npm install -g ember-cli@1.0.1
npm ERR! Error: version not found: ember-cli@1.0.1
npm ERR! at G:\Program
Files\nodejs\node_modules\npm\lib\cache\add-named.js:
125:12
npm ERR! at saved (G:\Program
Files\nodejs\node_modules\npm\node_modules\npm
-registry-client\lib\get.js:167:7)
npm ERR! at Object.oncomplete (fs.js:107:15)
npm ERR! If you need help, you may report this entire log,
npm ERR! including the npm and node versions, at:
npm ERR! http://github.com/npm/npm/issues

npm ERR! System Windows_NT 6.1.7601
npm ERR! command "G:\Program Files\nodejs\node.exe" "G:\Program
Files\nod
ejs\node_modules\npm\bin\npm-cli.js" "install" "-g" "ember-cli@1.0.1"
npm ERR! cwd G:\Projects\package-ui
npm ERR! node -v v0.10.31
npm ERR! npm -v 1.4.23
npm ERR!
npm ERR! Additional logging details can be found in:
npm ERR! G:\Projects\package-ui\npm-debug.log
npm ERR! not ok code 0

On 28 November 2014 at 01:07, Jonathan Goldman notifications@github.com
wrote:

It's a Thanksgiving Miracle!! [image: 🎉] [image: 🎈] [image:
🎉] [image: 🎈]


Reply to this email directly or view it on GitHub
#1 (comment)
.

Bryan Crotaz
Managing Director
Silver Curve

@rwjblue
Copy link
Member

rwjblue commented Nov 28, 2014

@Twinkletoes - It isn't a new version of ember-cli, just symlink-or-copy. We need to update the bundled dep in in ember-cli before pushing a new version.

@BryanCrotaz
Copy link

Ah - how do I install this into my local ember-cli installation?

On 28 November 2014 at 01:15, Robert Jackson notifications@github.com
wrote:

@Twinkletoes https://github.com/Twinkletoes - It isn't a new version of
ember-cli, just symlink-or-copy. We need to update the bundled dep in in
ember-cli before pushing a new version.


Reply to this email directly or view it on GitHub
#1 (comment)
.

Bryan Crotaz
Managing Director
Silver Curve

@Xylez01
Copy link

Xylez01 commented Nov 28, 2014

My team and I are going to love this, great work!

@karl-sjogren
Copy link

@Twinkletoes Remove your node_modules/-folder and do a npm install. Since the dependency for embercli is written as "symlink-or-copy": "^1.0.0" it will take the 1.0.1 version when reinstalling. It might not be enough for other modules though if they are matching 1.0.0 specifically.

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.