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

Build the start script path correctly on Windows #685

Merged
merged 4 commits into from
Apr 19, 2016
Merged

Build the start script path correctly on Windows #685

merged 4 commits into from
Apr 19, 2016

Conversation

jendib
Copy link
Contributor

@jendib jendib commented Apr 17, 2016

@johnnyman727
Copy link
Contributor

@jendib thanks for taking the initiative to fix this.

Could you explain why the path.join method doesn't work on Windows? What path does it create? I was under the impression that the purpose of path.join was to abstract different file system implementations across platforms.

@jendib
Copy link
Contributor Author

jendib commented Apr 18, 2016

Yes path.join indeed abstract the host FS, however in this case, we don't want this. We want to create the path to the init script used to launch the node app when Tessel is starting. So we need to build a unix path.
Before, this line does this: \app\start
And after: /app/start

@rwaldron
Copy link
Contributor

@johnnyman727 this is correct, for the same reason as: 50732a7 I'll spend some time today checking for anymore mistakes like this

@jendib two things:

  • based on the commit I linked above, do you think you can write a test for this change? If you need help, ping me here or on slack—I'd be happy to help get you started
  • If you haven't done so already, please sign the CLA: http://dojofoundation.org/about/claForm

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
@rwaldron
Copy link
Contributor

@jendib and I discussed a strategy for tests, all that remains now:

Once those are complete, and this PR is green, then it's +1 to merge

Assert that shellScriptPath is not normalized for the local environment.
@jendib
Copy link
Contributor Author

jendib commented Apr 18, 2016

It's merged and I signed the CLA, thanks @rwaldron !

@brunohunziker
Copy link

Could somebody make a new relaease containing the change?

@@ -1035,7 +1035,7 @@ actions.pushScript = function(t, script, opts) {
actions.writeToFile = function(t, entryPoint) {
return new Promise(function(resolve, reject) {
// Path of the script to run a Node project
var shellScriptPath = path.join(Tessel.REMOTE_PUSH_PATH, PUSH_START_SCRIPT_NAME);
var shellScriptPath = Tessel.REMOTE_PUSH_PATH + PUSH_START_SCRIPT_NAME;
Copy link
Contributor

Choose a reason for hiding this comment

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

You could change this into path.posix.join(...) instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, thanks!

@rwaldron rwaldron merged commit 4432036 into tessel:master Apr 19, 2016
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.

None yet

5 participants