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

test: fix test-require-symlink on Windows #23691

Merged
merged 1 commit into from
Oct 23, 2018

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Oct 16, 2018

Creating directory symlinks on Windows require 'dir' parameter to be provided.

Fixes: #23596

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 16, 2018
@bzoz
Copy link
Contributor Author

bzoz commented Oct 16, 2018

@refack
Copy link
Contributor

refack commented Oct 16, 2018

Shouldn't we validate the arg and throw, or assume dir?
This exposes an API asymmetry between platforms.
.
.
.
@bzoz could you add a new failing test to known_issues?

@refack refack added fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform. labels Oct 16, 2018
@bzoz
Copy link
Contributor Author

bzoz commented Oct 16, 2018

@refack the API assumes 'file', using 'dir' makes libuv use SYMBOLIC_LINK_FLAG_DIRECTORY with CreateSymbolicLinkW.

We could add a test to see if link target is a directory and make it automatically use 'dir'.

@refack
Copy link
Contributor

refack commented Oct 16, 2018

We could add a test to see if link target is a directory and make it automatically use 'dir'.

👍 (or throw)

@bzoz
Copy link
Contributor Author

bzoz commented Oct 23, 2018

Resumed CI, all green: https://ci.nodejs.org/job/node-test-commit/22568/

@refack
Copy link
Contributor

refack commented Oct 23, 2018

Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: nodejs#23596

PR-URL: nodejs#23691
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack refack force-pushed the bartek-fix-test-require-symlink branch from 0cbd76c to d1d5924 Compare October 23, 2018 15:19
@refack refack merged commit d1d5924 into nodejs:master Oct 23, 2018
targos pushed a commit that referenced this pull request Oct 24, 2018
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bzoz added a commit to JaneaSystems/node that referenced this pull request Nov 29, 2018
On Windows creating a symlink to a directory will not work unless extra
'dir' parameter is passed. This adds a check if link target is a
directory, and if so automatically use 'dir' when creating symlink.

Ref: nodejs#23691
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 29, 2018
On Windows creating a symlink to a directory will not work unless extra
'dir' parameter is passed. This adds a check if link target is a
directory, and if so automatically use 'dir' when creating symlink.

PR-URL: nodejs#23724
Refs: nodejs#23691
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
On Windows creating a symlink to a directory will not work unless extra
'dir' parameter is passed. This adds a check if link target is a
directory, and if so automatically use 'dir' when creating symlink.

PR-URL: nodejs#23724
Refs: nodejs#23691
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test-require-symlink error on Windows 10
6 participants