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

Proper use of path.join(..., ...) and path.sep to be more cross-platform. #303

Closed
wants to merge 1 commit into from

Conversation

@TaraWij
Copy link
Contributor

@TaraWij TaraWij commented Aug 26, 2012

This pull request features a lot of line changes to fix occurences where the / character was used explicitly, as well as to fix occurences of ../ and ./ to use __dirname instead where possible.

Node.js introduces path.sep in 0.8.x, which allows us to replace the / used in matches; however

/home/tomwij/Downloads/meteor/dev_bundle/bin/node: symbol lookup error: /home/tomwij/Downloads/meteor/dev_bundle/lib/node_modules/fibers/src/fibers.node: undefined symbol: _ZNK2v85Value11IsUndefinedEv

It appears Node.js 0.8.x doesn't go together with Fibers, hence I need help for others to confirm this is indeed a problem; we should then need to urgently report this to get a timely fix, else we can't bump across 0.8.x in the near future, unless the fibers fix would be trivial.


The reason I am putting this pull request early is to let you know it is there and such that inspection can happen early, as I feel this is a large code change it shouldn't be pulled in right away, but thorougly checked.

One example thing I need to do is to check for each file that I have modified whether there is a var path = require('path'); in place and that it is above any other references to path. Furthermore, __dirname can't be used in the Fibers context, so that needs to be removed from any packages/ files. And a very last thing is that there were one or two occasions were I wasn't sure whether it was a path or not (eg. the serve_root variable), the solution for this last issue has two options:

  1. Replace those instances back to /, but possible replaces need to be done when translating paths to URLs.
  2. Translate all URLS to use / instead of \ right before passing it on to functions that parse the URL.

I expect three more commits; one fixing the calls to a non-defined path and __dirname, one fixing broken URLs and possibly one to support Fibers and fix remaining bugs.

Hopefully this will help us to get towards a more cross-platform Meteor, reaching to not only the main OSes but perhaps even beyond. Thank you in advance... :)

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Aug 28, 2012

Thanks! Appreciate the early heads up. This looks good. Do I understand right that it requires node 0.8 before it can be merged?

@TaraWij
Copy link
Contributor Author

@TaraWij TaraWij commented Aug 28, 2012

Exactly, and Meteor doesn't work with 0.8.x for me due to Fibers.

So, we first need Fibers in order then correct it further and then do extensive testing before we can pull this in.

Yet, I already did the pull request to know it's there and easier tracking...

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Sep 4, 2012

We've got meteor working with node 0.8 on the node-0.8 branch. A little more cleanup and testing, and we'll be ready to merge to devel.

@TaraWij
Copy link
Contributor Author

@TaraWij TaraWij commented Sep 6, 2012

@n1mmy: Good news from my side; have upgraded to 0.8.2 and installed the latest Fibers and have continued testing. It appears that after one small fix the three examples work perfectly to me; so, the only work left to do is verify whether all the packages work fine...

@glasser
Copy link
Member

@glasser glasser commented Sep 27, 2012

Yay, now that the 0.8-based Meteor release (0.4.1) is out, we can work on this. However, the pull request can't be automatically merged, so I guess you need to update it?

@TaraWij
Copy link
Contributor Author

@TaraWij TaraWij commented Sep 27, 2012

I'm right now merging in meteor/devel and checking if there are any additional changes I need to perform, moment.

@TaraWij
Copy link
Contributor Author

@TaraWij TaraWij commented Sep 27, 2012

Done now, merged in latest, resolved conflicts, checked for typos and ensured the packages should work...

You should now be able to automatically merge this pull request.

@TaraWij
Copy link
Contributor Author

@TaraWij TaraWij commented Oct 25, 2012

Please defer pulling this until I brought out 0.5.0 for Windows which will be based on this branch instead of the prior pull request, this will ensure that these changes are complete.

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Oct 25, 2012

Okie dokie. I did a little work testing this on the path-fixup-303 branch, but I'll wait until win 0.5.0 to finish and merge. Thanks.

@TaraWij
Copy link
Contributor Author

@TaraWij TaraWij commented Oct 25, 2012

It appears I left one other bug in this line (missing path.join) which I discovered while attempting to deploy, it appears I didn't try that earlier on that branch. Do you want me to redo the commit with your fixes (and a fix for this line) merged in so you can merge in a single commit?

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Oct 25, 2012

One commit with all the fixes based on a recent devel would be great.

@TaraWij
Copy link
Contributor Author

@TaraWij TaraWij commented Oct 30, 2012

@n1mmy: Done, based on latest devel, is one commit and includes the patch from your branch.

All tests pass on Windows except for the sass test, although the parser does something so it's probably just a small bug.

@n1mmy n1mmy closed this in 634fb11 Oct 31, 2012
@n1mmy
Copy link
Member

@n1mmy n1mmy commented Oct 31, 2012

Merged to devel! Thanks for all your work on this, and your patience on getting it merged =)

for the logs, I tested:

  • unit tests
  • running an app w/ bootstrap
  • running the docs (showdown, code-prettify)
  • upgrade / post-upgrade (built a release package, ran upgrade on both mac and linux)
  • cli-tests.sh
  • deploying apps
  • incrementing version numbers

I think that covers all the changed code...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.