Issue #264: Fixes for windows paths #460

Closed
wants to merge 2 commits into
from

Projects

None yet

6 participants

@mridgway
Collaborator
mridgway commented Sep 4, 2012

This PR gets mojito up and running on Windows (tested on Windows 7 with Node 0.8.6). I tested against the "newsboxes" example app and all seems to be working.

Known issues:

  • Unit tests need to be fixed so that they pass on a Windows box, although they still pass on our dev and build boxes
  • mojito jslint's exclusion matcher uses regular expressions that contain "/". It may be better to use glob to get a list of exclusions instead.
  • Other command line tools haven't been tested

I'm sure there's more, but this should at least get the ball rolling for anyone out there that is interested in Windows support.

@mridgway mridgway commented on an outdated diff Sep 4, 2012
lib/store.server.js
@@ -991,6 +992,7 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
if (!fs.isFile && fs.subDirArray.length < 2 && 'addons' === fs.subDirArray[0]) {
return true;
}
+
@mridgway
mridgway Sep 4, 2012 collaborator

This new line made it in because of a console.log testing. I'll remove this.

@travisbot

This pull request passes (merged 4fd0351 into 7558065).

@drewfish drewfish and 1 other commented on an outdated diff Sep 4, 2012
lib/app/addons/rs/url.server.js
@@ -202,7 +207,7 @@ YUI.add('addon-rs-url', function(Y, NAME) {
rollupParts.push(this.config.appName);
}
rollupParts.push('rollup.client.js');
- rollupFsPath = libpath.join(this.appRoot, 'rollup.client.js');
@drewfish
drewfish Sep 4, 2012 Yahoo Inc. member

This seems very strange to me. My understanding was that the path library was supposed to do filesystem path construction in a way appropriate for each platform.

@mridgway
mridgway Sep 4, 2012 collaborator

My understanding is that the URL addon is in charge of generating URLs, not file system paths. As such, we should not be using libpath here at all, or else we get URLs with backslashes.

@drewfish
drewfish Sep 4, 2012 Yahoo Inc. member

That's mostly true. However, the rollupFsPath variable contains a path on the filesystem, and so should probably be using libpath.

@mridgway
mridgway Sep 4, 2012 collaborator

Got it. I'll change these fsPaths back to how they were before.

@drewfish drewfish and 1 other commented on an outdated diff Sep 4, 2012
lib/app/addons/rs/url.server.js
@@ -176,6 +176,11 @@ YUI.add('addon-rs-url', function(Y, NAME) {
rollupParts = [],
rollupFsPath;
+ // Fix windows paths
+ if ('win32' === process.platform) {
@drewfish
drewfish Sep 4, 2012 Yahoo Inc. member

Is there a way that the right thing can happen without the process.platform check? In this case, can the source of relativePath be fixed to not have the wrong path separator?

@mridgway
mridgway Sep 4, 2012 collaborator

On Windows, relative path contains backslashes, so in order to generate an appropriate URL, those backslashes need to be converted to forward slashes. I don't know of another way to do this. Theoretically you don't need the Windows check, so we could probably just remove the conditional.

@drewfish
drewfish Sep 4, 2012 Yahoo Inc. member

Yeah, it took me a bit to realize it was translating from FS path to URL. Perhaps the comment should say that instead: // translate from filesystem path to URL.

@drewfish drewfish commented on an outdated diff Sep 4, 2012
lib/app/addons/rs/url.server.js
@@ -213,7 +218,7 @@ YUI.add('addon-rs-url', function(Y, NAME) {
if (res.yui && res.yui.name) {
rollupParts.push(res.mojit);
rollupParts.push('rollup.client.js');
- rollupFsPath = libpath.join(mojitRes.source.fs.fullPath, 'rollup.client.js');
+ rollupFsPath = [mojitRes.source.fs.fullPath, 'rollup.client.js'].join('/');
@drewfish
drewfish Sep 4, 2012 Yahoo Inc. member

Ditto here, this is also a filesystem path.

@travisbot

This pull request fails (merged 7a1b21c into 7558065).

@rwaldura
rwaldura commented Oct 8, 2012

this PR is for issue #264: Windows support

@lzhan lzhan was assigned Dec 5, 2012
@lzhan
lzhan commented Dec 5, 2012

I will find a window machine to test and merge this pr.

@caridy
Collaborator
caridy commented Jan 29, 2013

@jenny @lzhan can we get rid of this? :)

@caridy caridy referenced this pull request May 8, 2013
Merged

making mojito to work on windows #1103

5 of 5 tasks complete
@caridy
Collaborator
caridy commented May 17, 2013

Closing after merging #1103

@caridy caridy closed this May 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment