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

Symlinks as symlinks #131

Closed
wants to merge 24 commits into from
Closed

Conversation

LMnet
Copy link

@LMnet LMnet commented Jan 16, 2014

This pull request can really closed this issue.

@vladikoff
Copy link
Member

Thanks for the pull request, could you please squash the commit into one commit.


If set to false, `dest` file will contain data from the `src` symlink's target.

Under Windows, the option has no effect, i.e. is always `false`.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this? Can Windows not copy symlinks or what is the issue?

Copy link
Author

Choose a reason for hiding this comment

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

Windows have a lot of problems with symlinks, it doesn't work properly.

Copy link
Member

Choose a reason for hiding this comment

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

This needs to work on Windows to merge. What issues do you have with symlinks on Windows? What version of Windows?

Copy link
Author

Choose a reason for hiding this comment

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

Only Windows Vista and above can handle symlinks. And git clone transform symlinks on windows into the regular files. I need some time to solve this problems.

@shama
Copy link
Member

shama commented Jan 28, 2014

This seems like something that should belong in grunt.file.copy instead of just implemented in this task. /cc @tkellen

@LMnet
Copy link
Author

LMnet commented Jan 28, 2014

This seems like something that should belong in grunt.file.copy instead of just implemented in this task.

I agree with this. But now, grunt.file.copy can't copy symlinks, and I found, that fixing grunt.file.copy is a lot more complex goal. In my project (I'm using grunt to build desktop C++ application) I really need to copy symlinks as symlinks on linux. I decided to take the easy way: fix the problem in this repo.

@mr-moon
Copy link

mr-moon commented Feb 6, 2014

It will fail, if symlink links to a directory.

@mr-moon
Copy link

mr-moon commented Feb 6, 2014

@LMnet I've made a pull request to your fork that adds support of copying symbolic directory links mr-moon@ce599e9

@LMnet
Copy link
Author

LMnet commented Feb 9, 2014

I finally created workaround for Windows. I tested on Windows XP, Windows 7 and Ubuntu Linux, and all tests are succesfully passed.
I'm waiting for review. After that I will squash commits.

grunt.file.mkdir(dest);
if (options.copySymlinkAsSymlink && isLink) {
try {
fs.symlinkSync(fs.readlinkSync(src), dest);

Choose a reason for hiding this comment

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

Needs to create parent dir, delete link if it exists, as below:

  grunt.file.mkdir(path.dirname(dest));
  if (grunt.file.exists(dest)) {
    grunt.file.delete(dest);
  }

Copy link
Author

Choose a reason for hiding this comment

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

I don't fully understand what you mean. Can you give some more details?

Choose a reason for hiding this comment

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

Sure, two cases, both start with a symlink /foo/bar to a directory /baz/dir/ that we want to copy to /beep/boop:

  1. When /beep/boop already exists copy will fail. If bar were a symlink to a file instead of a directory, the code below would delete the existing symlink, /beep/boop/ before creating the new one, and pass. Seems like behavior should be consistent either way. I'd prefer always delete, then create.
  2. When neither /beep/ nor /beep/boop exist, copy will fail trying to write boop to the non-existant /beep/ directory. Again if bar linked to a file instead of a directory, we'd first create /beep/ then copy in boop. Should also be consistent.

@shama
Copy link
Member

shama commented Feb 11, 2014

I still think this belongs in grunt.file.copy instead of this task. /cc @tkellen @cowboy

@mr-moon
Copy link

mr-moon commented Feb 11, 2014

@shama we still agree with you. You may just leave this pull open, so others, who fall into same circumstances as we, may still fork and benefit from it.

@tkellen
Copy link
Member

tkellen commented Feb 13, 2014

@shama If we can get a PR in for this on Grunt I will merge and publish it as a patch release.

@vladikoff
Copy link
Member

@shama should this go into grunt or grunt-next?

@LMnet it might make it easier for others to use this if you squash this. Make sure to not delete your branch.

@vladikoff vladikoff closed this Feb 26, 2014
@shama
Copy link
Member

shama commented Feb 26, 2014

@vladikoff grunt, hopefully soon I'll get around to a pull request. I'm completely overloaded atm.

@jamesplease
Copy link
Member

This issue references #30, but they seem to have two different goals. What I gather from #30 is that the user wants to be able to generate symlinks from actual files (which I suppose is now accomplished by grunt-contrib-symlink), but this issue seems to discuss the problem of copying a symlink as a symlink, instead of creating a new file from a symlink.

The latter issue is really the problem we're trying to solve, right?

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.

An option to generate symlinks?
8 participants