Private public overlap

[rgrinberg]

This is a more correct fix for #580. I might have done it wrong, but this fix still isn't complete. It will still generate an incorrect META if you do so without building the actual library. This is possible by requesting the META target directly or having a library defined without any modules. Do we need both fixes, or somehow tweak this one to cover the META case?

Also, I. I'm not generate a good error value so I just a dumb thing that will work. Would like to know if there's a proper way to do this.

[ghost]

I think the check should go in the resolve_simple_deps and resolve_complex_deps functions instead of closure. This way errors will be caught immediately. Additionally, we still have the location in these two functions, so we can point the error to the invalid dependency in the jbuild file.

[rgrinberg]

What about adding this check into resolve_dep so that I don't have to do it twice in those 2 functions?

[ghost]

That seems fine. That's better actually, since it should also catch issues with sub-systems such as inline tests trying to use a private library from a public one

[rgrinberg]

Hmm, resolve_dep isn't enough either because find_internal can simply hit the cache and ignore this value I think. So I think we gotta add this check to find_internal, but then we don't have the ~loc again...

It's a shame we can't update the database itself somehow to exclude the private dependencies. This would save us having to thread this annoying parameter everywhere. But then proper location reporting is still an issue.

[rgrinberg]

Nvm, I ended up fixing the problem. I just needed to detect the allow_private_deps status directly in instantiate. But still, passing around this param everywhere isn't pretty.

[rgrinberg]

On second thought, I think this approach with allow_private_deps doesn't really work when we take preprocessors into account. Private preprocessors may be introduced in public libraries as long they don't have any private runtime dependencies.

How about we doing the following: we verify the list of dependencies only in instantiate. That seems like it should work. I've pushed a small prototype (see the last commit)

[ghost]

Checking in resolve_dep seems better to me. For the preprocessors, we just need to selectively do this check in resolver_user_deps, i.e. we don't do it for preprocessors but we do it for runtime deps. Why does find_internal needs to know about allow_private_deps? Technically what we need to do is resolve the dependency name, and once it's resolved then we do the check.

[rgrinberg]

I'm not entirely sure about find_internal. I did gave that approach a try but it didn't work for me. IIRC, for a preprocessor we are allowed private dependencies to build it, but we still cannot introduce private runtime dependencies. And that case never ended up working.

I'm not too sure about doing it instantiate, but passing a flag to resolve_dep (or anywhere else) is also suspicious to me. The issue is that we shouldn't really need the caller to pass a flag for Lib to know whether private dependencies are applicable. We should know this by detecting the status of the library itself. So I how about I do the verification of lib.requires and lib.ppx_runtime_deps in the St_found clause whenever the library is private.

[rgrinberg]

Closed in favor of #607.