linkall is POSIX 2008 and I'm not sure it's available everywhere. You need to guard its use with

#if _XOPEN_SOURCE >= 700 || _POSIX_C_SOURCE >= 200809L

and provide an #else implementation that uses link and fails in the cases that link cannot handle.

Also, some motivation on why we really need to hardlink symlinks would be welcome.

If I'm reading the binding correctly, ?follow defaults to false which is the opposite of the previous default:

link() will resolve and follow symbolic links contained within both path1 and path2.

Additionally, at least on OS X,

On OS X, not assigning AT_SYMLINK_FOLLOW to flag may result in some filesystems returning an error.

Why not instead add Unix.linkat (with relevant configure-based detection for the underlying C function)?

Concerning the "bug fix" I confirm there is no bug and the fix would break the win32 code.

@madroach : if you want to keep this PR up for discussion, please remove the 'bug fix", correct the default behavior of Unix.link, and add a fallback implementation if linkat() is not available. Otherwise we'll close this PR.

I updated the pull request in the following ways:

• removed my supposed bug fix in the win32 symlink code. I missed out on the option already being handled in unix.ml. There is no bug.
• I also added checks for __POSIX_VERSION and _XOPEN_VERSION version to use simple link as fallback.
• The default behavior if no ~follow parameter is supplied is now following symlinks.

On the behavior of link the linux manpage on link(2) says this:

POSIX.1-2001 says that link() should dereference oldpath if it is a symbolic link. However, since kernel 2.0, Linux does not do so: if oldpath is a symbolic link, then newpath is created as a (hard) link to the same symbolic link file (i.e., newpath becomes a symbolic link to the same file that oldpath refers to). Some other implementations behave in the same manner as Linux. POSIX.1-2008 changes the specification of link(), making it implementation-dependent whether or not oldpath is dereferenced if it is a symbolic link. For precise control over the treatment of symbolic links when creating a link, see linkat(2).

Also http://pubs.opengroup.org/onlinepubs/9699919799/functions/link.html may be of interest.

I have no strong preference, but the not dereferencing approach seems saner to me because one could still dereference the symlink with readlink(2) and then create a hardlink to the target file.

As to why this change is needed:

Creating hardlinks of symlinks is error prone:

• when following the symlink and the symlink points into another filesystem, the hardlink will fail with EXDEV
• when not following the symlink and the symlink is relative and the new hardlink resides on a different directory than the symlink, the new (hardlinked) symlink will probably be broken and trying to access it will fail with ENOENT

Therefore the programmer often will need some control on what behaviour he prefers.
I need this feature for implementing a hardlinking backup solution like storeBackup or Apple Time Machine.
I could work around it by recreating new symlinks, so I don't depend on it. Still the cleaner solution would be hardlinking.

Thanks for the revised code, much appreciated. I have two more suggestions based on the discussion above and on the code:

• I would prefer that Unix.link behaves exactly like before if the ~follow: optional argument is not given. Since the follow/nofollow behavior of link() seems to depend on the target platform, the only way to achieve this compatibility is by calling link() and not linkat() when ~follow is not given.
• If ~follow: is given and linkat() is not available, the current code silently falls back to using link(), while I think it should fail.

Taken together, this would give a different code logic such as:

   int ret;
   caml_unix_check_path(path1, "link");
   caml_unix_check_path(path2, "link");
   p1 = caml_strdup(String_val(path1));
   p2 = caml_strdup(String_val(path2));
   caml_enter_blocking_section();
   if (follow == Val_int(0)) {
     ret = link(p1, p2);
   } else {
#if _XOPEN_VERSION >= 700 || _POSIX_VERSION >= 200809L
     int flags = Bool_val(Field(follow, 0)) ? AT_SYMLINK_FOLLOW : 0;
     ret = linkat(AT_FDCWD, p1, AT_FDCWD, p2, flags);
#else
     errno = ENOSYS /*or something more appropriate*/; ret = -1;
#endif
   }

Thanks for your input. This is indeed the most sensible behaviour. I adapted the code as you suggest, but kept the parsing of the option out of the blocking section because I'm not sure whether access to the option is protected from interference by the garbage collector.

It would be great to have the documentation updated to describe the ?follow behavior including None ➡️ link() and Some (true | false) ➡️ linkat().

documentation added.

What about this interface?

type link_flag =
|L_FOLLOW

val fd_cwd:dir_handle

val linkat ~flags:link_flag array ->
  ?src_dir:dir_handle -> ~src:string ->
  ?dst_dir:dir_handle -> ~dst:string -> unit

But on the other hand the ?follow:bool -> string -> string -> unit interface won't break anything and we can still add linkadd in case more flags are needed.

I agree that the ~follow parameter feels an easy way of adding this, but it also (to me) encodes far too much important (user) consideration into the two constructors of bool option (even though you've documented it). Upon further reflection, I also dislike the Windows treatment - at present, hardlinking to a symbolic link on NTFS is a contradiction in terms, so this parameter should not be ignored on Windows, it really ought to be an exception if follow is false.

I would push for the separate interface (if a user - such as yourself - needs the version with optional argument, it is easy to define) but with the following changes:

• No need to expose fd_cwd - it's well-encoded as the default for ?src_dir & ?dst_dir and can be compatibly added in future if some extra special values appear (as you would still want the default to be AT_FDCWD)
• There are of course other *at functions (e.g. fstatat(2)) which might want binding in future. Given the way these flags work, this could well be a case to use a polymorphic variant. Alternatively, the type should be better named (e.g. at_flags with an AT_) prefix. The polymorphic variant would be superior as it would allow each at function to specify which flags are permitted and which aren't, obviously.
• I assume that the labels would be for UnixLabels only (I think that's the "policy" even for new functions)

I think this PR is getting out of hand and turning into a design discussion for a much bigger extension than initially offered. In particular, if linkat is exposed in its full generality, the other *at functions should also, and that's not a decision to be taken lightly because the *at functions have no Win32 counterpart as far as I know.

I updated the pull request to include most suggestions by dra27.

According to CreateHardLink the only behavior on win32 is following hardlinks.
Therefore the win32 port will now fail with NOSYS if ~follow:false is requested. The unix port will fail in the same way if linkat is unavailable and any specific ~follow behavior is requested.

Since we are talking about the *at functions, last year I wrote a bindings or them. This will be released in the main opam repository this week

@dra27 @xavierleroy can we get consensus on whether to include this?

What can I say?

1. The API is fine.
2. The implementation is almost good, see my suggestion above.
3. I'm still not sure how much we care about this issue. Hard links are very rarely used nowadays.

@madroach if you fix the int flags issue and rebase, we'll merge this PR.

done and rebased.

fixed Changes

The CI reports failing macos-10.3 and freebsd builds with the same error:

gcc -c -O2 -fno-strict-aliasing -fwrapv -Wall -Werror   -D_FILE_OFFSET_BITS=64 -D_REENTRANT -DCAML_NAME_SPACE  -I../../byterun -o link.o link.c
link.c:45:15: error: use of undeclared identifier 'errno'
    ret = -1; errno = ENOSYS;
              ^
link.c:45:23: error: use of undeclared identifier 'ENOSYS'
    ret = -1; errno = ENOSYS;
                      ^
2 errors generated.

Seems like I used the feature test macros in the wrong way.

It may be a missing #include <errno.h> in otherlibs/unix/link.c.

A fix is proposed in #1862