html-deps: Fix package flatness assumption. #307
Conversation
| let elts = Sys.readdir (to_string t) |> Array.to_list in | ||
| List.fold_left elts ~init:[] ~f:(fun acc elt -> | ||
| let file = File.create ~directory:t ~name:elt in | ||
| if Fpath.is_file_path file then file :: acc else acc |
There was a problem hiding this comment.
If anyone wonders w.r.t. what I'm replacing with that line was absurd. Fpath works syntactically it nevers queries the OS.
html-deps now looks recursively in the package directory for odoc files.
dbuenzli
force-pushed
the
fix-html-deps
branch
from
25423fc
to
9bf93f1
Compare
Is this right? I would have expected to create a flat structure for the odoc files, the structure has no meaning for them. |
The problem is that nothing prevents a package from installing a cmti file with the same name in different parts of their hierarchy. |
|
But IIRC then we can't generate the html for that package anyway, right? |
You can. See https://github.com/ocaml/odoc/blob/master/src/odoc/env.ml#L20-L35 |
|
But don't the names of the html files for the modules collide and override each other? |
Maybe I don't know about the details, I'll let people who work on odoc figure it out. I'm working this way for now in In any case I don't think looking things up recursively in the directory is harmful, if you generate things flatly no cost is incurred by this patch, so I'm not sure what your objection is here. Besides you could also imagine a tool that works directly in the (Not that I think that |
|
The point of that bit in Env was to not die horribly, rather than to actively support the idea that having module name clashes within a package is a good idea. The use in OCaml itself is an interesting one - threads and vmthreads are mostly interchangable, but not quite; so we can't quite use the virtual libraries idea without a bit of rearragement. @dbuenzli - do you happen to know if this is the only clash? |
No objection, I was just surprised that it was needed. |
There are others like Regarding In other words I'm not sure I arealdy saw such a second case but regardless, I'll proceed that way for the time being. It will be a single line change in |
|
The code looks good, but I defer to @lpw25, @dbuenzli, @jonludlam on whether recursive scan is justified. |
|
Merging this. No strong objection it seems. Also useful in build scenarios where your might want to generate your |
|
Meh. Independently of whether people think there should be a flat structure to store odoc files or not, one thing that I don't understand is why you can't just pass the right include dirs to odoc. Btw:
In a case like this I think you're introducing issue by making the search recursive. And the example of "well ocaml has such a structure" is not convincing, or rather, I see it as a good example that you shouldn't do this! For example, ocaml expects you to pass a Does this make sense or did I misunderstand what's going on here? |
There is no include option in the Again I would have found it much more natural for
I don't think it will introduce any issue. It will just mix the dependencies of all the odoc files as
Of course you can only link with only one of these units name. But a package may install more than one unit with the same name; if those have different documentation how do you document them ? One answer I'd be perfectly happy with would be this is illegal; but so far that's has not been the case. |
|
OK so I had just forgotten how |
Make
html-depslook recursively in the package directory for odoc files. Before this patch it only looked at the first level of the given package directory which is not what you want in practice.A lot of packages have a hierarchy in their install libdir (try e.g.
ls $(opam var odoc:lib)). When you generate theodocfiles you copycat that hierarchy. As mentionedhtml-depswould not lookup the hierarchy but only the first level of the hierarchy. For a package likeodocitself this would return an empty set of deps, which would then result in failures further down the pipeline.