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

CB-14033 Support symbolic dir links on Windows #621

Merged
merged 1 commit into from Jul 12, 2018

Conversation

@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Jun 27, 2018

Platforms affected

Windows

What does this PR do?

This uses junctions whenever creating directory symlinks on Windows since that does not require any special privileges.

What testing has been done on this change?

Tests on AppVeyor CI

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.
@raphinesse raphinesse force-pushed the raphinesse:junctions branch from d81f242 to 8d91ba1 Jun 27, 2018
@raphinesse raphinesse changed the title CB-14033 Support symbolic links on Windows CB-14033 Support symbolic dir links on Windows Jun 27, 2018
@raphinesse raphinesse force-pushed the raphinesse:junctions branch from 8d91ba1 to e9f916f Jun 28, 2018
@raphinesse raphinesse requested review from dpogue and brodybits Jul 4, 2018
@brodybits
Copy link
Contributor

@brodybits brodybits commented Jul 4, 2018

I think it would be best to test with Cordova CLI, would be ideal to test with plugin which may be affected by this change. I can take a better look tomorrow.

@dpogue
dpogue approved these changes Jul 4, 2018
Copy link
Member

@dpogue dpogue left a comment

I haven't tested this, but the changes look good to me

@raphinesse
Copy link
Contributor Author

@raphinesse raphinesse commented Jul 4, 2018

@brodybits Since there is a test that specifically targets the changed code, I'm quite confident it's OK.
However, some real-world testing is always good. Thanks for taking care of it 👍

@raphinesse
Copy link
Contributor Author

@raphinesse raphinesse commented Jul 11, 2018

@brodybits Did you test this on Windows yet? Or did I misunderstand your comment? 🤔

@brodybits
Copy link
Contributor

@brodybits brodybits commented Jul 11, 2018

My apologies, no time for me to test it this week, probably not much time next week either. Considering that you tested it in spec and got an approval from someone else I don't think you should let me block it.

I do think it would be good for you to try a version of cordova-cli that does use a version of cordova-lib with this change and verify that it does actually solve the problem at some point.

This uses junctions whenever creating directory symlinks on Windows
since that does not require any special privileges.
@raphinesse raphinesse force-pushed the raphinesse:junctions branch from e9f916f to 75f1d63 Jul 11, 2018
@raphinesse raphinesse merged commit 17f5b7f into apache:master Jul 12, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@raphinesse raphinesse deleted the raphinesse:junctions branch Jul 12, 2018
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