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

Scan include directories for odoc files eagerly #268

Closed
wants to merge 6 commits into from

Conversation

jonludlam
Copy link
Member

Goes some way to addressing #148.

This is an RFC, particularly the error handling. As is, this still results in a nasty backtrace spewed to the terminal so I don't think it should be merged yet. Before I just wrapped the invocations of Env.create in an exception handler I thought I'd better ask whether there was a better plan in mind for this sort of non-recoverable one?

This allows early detection of problems such as there being two
files of the same name in different paths (see ocaml#148 for an example)

Signed-off-by: Jon Ludlam <jon@recoil.org>
Signed-off-by: Jon Ludlam <jon@recoil.org>
Signed-off-by: Jon Ludlam <jon@recoil.org>
@aantron
Copy link
Contributor

aantron commented Dec 7, 2018

We do eventually plan to have more disciplined error handling at the top level, but so far we've been working "bottom-up" by introducing proper error handling in the parser first, and gradually propagating new error handling up to the command-line interface level. This process has reached only the parser and parts of the loader so far, however.

I'd say to do whatever you think is easiest here :) It could be the right solution for the long term, but even if not, that's okay, because then we will still address it later :)

@jonludlam
Copy link
Member Author

OK. I'll stick with raising the exception from Model.Errors and catch it right at the top.

I'll need to stick in some more error handling around the eager loading too, since having just tested it on Coq I find it's failing with End_of_file, because dune is running a whole bunch of odoc instances in parallel all writing out odoc files and hence we can't guarantee to be able to read them.

Otherwise when executing a build in parallel we try to read in
incomplete odoc files even though we almost certainly don't
need to read them.

Signed-off-by: Jon Ludlam <jon@recoil.org>
Signed-off-by: Jon Ludlam <jon@recoil.org>
Signed-off-by: Jon Ludlam <jon@recoil.org>
@lpw25
Copy link
Contributor

lpw25 commented Dec 7, 2018

because dune is running a whole bunch of odoc instances in parallel all writing out odoc files and hence we can't guarantee to be able to read them.

You should be able to use the imports list to restrict the eager scanning to just those files we will actually use. This should remove issues with parallelism and also prevent non-reproducible documentation. Essentially you should only include files whose name matches those in the import list.

There could still be problems if there are two files with the same name, and dune thinks only one of them is a dependency. So some protection for End_of_file should still be there, but the problem should then be extremely rare and also a symptom of an actual problem with how the build system is handling dependencies.

@jonludlam
Copy link
Member Author

Ah, this sounds much better. I'll have a look.

@jonludlam
Copy link
Member Author

This is the wrong solution.

@jonludlam jonludlam closed this Dec 14, 2018
@aantron
Copy link
Contributor

aantron commented Dec 14, 2018

@jonludlam Could you link to something, or state in one brief sentence what the core of the reasoning is?

@jonludlam
Copy link
Member Author

I was planning on doing so in the follow-up PR, which I have cooking. I just didn't want to waste anyones time reading this when I'm intending to do something different

@aantron
Copy link
Contributor

aantron commented Dec 14, 2018

Fair enough, thanks.

jonludlam added a commit to jonludlam/odoc that referenced this pull request Dec 17, 2018
Should be very rare, see ocaml#268 for rationale

Signed-off-by: Jon Ludlam <jon@recoil.org>
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.

3 participants