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

Normalize paths for builds on windows #387

Closed
wants to merge 2 commits into from

Conversation

paulvarache
Copy link
Contributor

@paulvarache paulvarache commented Sep 10, 2018

Transforms backslashes to forward slashes to fix builds on windows

@coveralls
Copy link

coveralls commented Sep 10, 2018

Coverage Status

Coverage decreased (-0.1%) to 98.701% when pulling 5f93f57 on paulvarache:win-build into 34e2605 on Tonejs:dev.

@tambien
Copy link
Contributor

tambien commented Sep 10, 2018

thanks for the PR! i wonder if using path.posix is a more elegant approach here. Feels a little weird to be inspecting the platform and node provides a cross-platform way to generate unix-style paths.

does collect_tests.js also needs the same update to work on Windows? it does a similar thing as collect_deps.js

@paulvarache
Copy link
Contributor Author

I updated the PR to fix the tests on windows. The npm script is using the $npm_config_dir and $npm_config_file variables but this is not resolved on windows. I added a normalizeVariable method that deal with that, but could also use process.env to have a unified API across platform, but I'm not sure if the args file and dir of collect_tests.js are used anywhere else and don't want to break things.

I would be happy to use the file/dir arguments and merge them with $npm_config_dir and $npm_config_file env variables in the script. Updating the npm script collect:tests to not take any file or dir args from the unix shell. Something like this:

const file = args.file || process.env.npm_config_file;
const dir = args.dir || process.env.npm_config_dir;

As for the path resolution, I looked into posix, but it won't convert Windows style path into posix style paths, it is just two different implementations of the path module (see nodejs/node#12298).

@tambien tambien closed this in 8070135 Feb 2, 2019
@adenflorian
Copy link

@tambien
@paulvarache 's solution works for me (windows 10, node v10.14.1), but your commit 8070135 doesn't. There are a couple open issues on path.posix for windows:

When using posix, for me, it's outputting things like this into index.js when building:
require("./../../C:/Users/Me/Tone.js/Tone/component/Follower");

Think we could go with paul's solution for now?

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.

4 participants