fs.lchmod only available on OSX? #3598

Closed
ry opened this Issue Jul 1, 2012 · 9 comments

Projects

None yet

4 participants

@ry
ry commented Jul 1, 2012

We shouldn't have system dependent APIs like this.

See b9abb64, #853

@isaacs isaacs was assigned Jul 1, 2012
@isaacs

Sure, but Linux and SunOS don't have any concept of a symlink's stat or mode as apart from the target, as that's just a BSD-ism. (They also can't open(2) with O_SYMLINK.)

I'd rather not remove the ability to do this on OS X. Maybe we should just make lchmod/lchown a no-op on non-BSD systems?

@piscisaureus What about windows? We treat junctions and symlinks pretty similarly now. Can you set the mode and owner on a junction or symlink there?

@bnoordhuis
Node.js Foundation member

as that's just a BSD-ism.

I hate being pedantic (okay, not really - I just hate how people react) but O_SYMLINK is not a BSD-ism. XNU (that's OS X) is the only kernel that supports it.

@isaacs

@bnoordhuis Oh, ok. I think you were the one who told me that O_SYMLINK was a BSD-ism. I didn't check up on it, because you're usually so much more pedantic than me, so I just trusted you blindly ;)

In any event, I'm ok with no-op'ing these calls on systems that don't have the concept of a symlink as apart from its target, but we do need to still maintain this functionality for OS X. Another option is to leave the methods missing (not ideal, but at least feature-detection-friendly), or raise ENOTSUP (which will probably break some stuff).

@bnoordhuis
Node.js Foundation member

IMO the best option is to leave them as-is but remove them from the documentation (and add a big DO NOT USE warning in the source). That should stop new people from using it without breaking existing code.

EDIT:

I think you were the one who told me that O_SYMLINK was a BSD-ism.

I don't remember that but maybe I misspoke.

@isaacs

But what if someone needs to set the mode or owner of a symlink on OS X? You need to be able to do this, since you can often end up with root-owned files in bad places otherwise. This was added to solve a real problem that npm faced, and other file-system utilities will encounter it as well. Putting a warning on it seems silly, since it's not actually bad at all, just un-portable.

It needs to be there because the functionality is necessary sometimes. (It's not necessary on Linux and SunOS.) It should be documented to be what it is. But having it randomly missing sometimes is weird, I agree.

What's wrong with making it a noop, and documenting the fact that it does nothing on other systems? Also, what about windows? Can you chmod or chown a symlink or junction there?

@ry
ry commented Jul 9, 2012

we should not have platform dependent API. we've gone to the ends of the earth to make this the case for 99% of API - and this seems like a silly thing to break the promise on.

@ry
ry commented Jul 9, 2012

it would be nice if npm didn't use symlinks (as i requested long ago...)

@isaacs

@ry Right, but that's not actually a reasonable approach in this case. As long as node provides any way to create or use symbolic links (ie, symlink, lstat, and readlink), then lchown and lchmod are necessary on OS X. Symbolic links are separate first-class files on OS X; it's annoying, I agree, but that's how it is.

You could say that the right answer is to not provide any way to interact with symlinks at all (no fs.symlink/lstat/readlink/etc), but the ship has sailed on that one, and turning it around is not possible. We've gone to the ends of the earth to make it work, even on Windows, and people have programmed accordingly.

it would be nice if npm didn't use symlinks (as i requested long ago...)

npm only uses symlinks to expose executables in the PATH on Unix, and does not allow symbolic links in package contents at all.

The alternative is shim files, and that's worse, though an unfortunate necessity on Windows, where there's no support for #! lines.

What you requested long ago was that npm not be used to put things in the PATH at all, but that was already not at option by that time.

@chrisdickinson

Closing due to inactivity. Let me know if I'm mistaken, and I can reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment