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

ocamldoc: keep using relative instead of absolute source file names #986

Merged
merged 1 commit into from Dec 26, 2016

Conversation

Projects
None yet
2 participants
@gasche
Member

gasche commented Dec 23, 2016

The fact that ocamldoc uses absolute file paths in its error messages
creates a usability problem for ocamlbuild users that see the path of
the files in _build instead of the path of the files in the source
project. See ocaml/ocamlbuild#79:

cd /tmp
mkdir repro; cd repro
echo "(** hey {ol {1 bla}} *)" > a.mli
echo "A" > doc.odocl
ocamlbuild doc.docdir/index.html

raises the error message:

File "/tmp/repro/_build/a.mli", line 0, character 11:
Error parsing text:
hey {ol {1 bla}}
           ^

and then people jump to the reported error site from their editor, and
then they are editing temporary _build files and it is a mess.

After the patch is applied, we get the following error message instead:

File "a.mli", line 0, character 11:
Error parsing text:
hey {ol {1 bla}}
           ^

The code that is removed by the patch is also a mess, of undocumented
assumptions. It is careful to Sys.chdir to the argument's directory,
and to capture a long name there and Sys.chdir back. This would make
perfect sense if other parts of ocamldoc were also using 'chdir' and
one could therefore not trust relative paths. But these were the only
two places using 'chdir', so the present change actually establishes
the property that relative paths can be trusted.

I am not imaginative enough to have a sense of what can go wrong as
we turn those absolute paths into relative paths. I looked at the
blame information, this code comes from the very first ocamldoc
commits by Maxence in 2002. My guess would be that early prototypes
used 'chdir' more agressively (maybe to call the typechecker as an
external command?), making absolute paths useful.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 24, 2016

Member

(cc: @Octachron; not because he should know, but because I would feel better if another not-knowning people would maybe also agree that we could try that.)

Member

gasche commented Dec 24, 2016

(cc: @Octachron; not because he should know, but because I would feel better if another not-knowning people would maybe also agree that we could try that.)

@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Dec 26, 2016

Contributor

Looking at ocamldoc code, I also have a hard time to see where things could go wrong. As far as I can see, file names are not really used outside of odoc_ast.ml and odoc_sig.ml − there seems to be one use in the odoc_dot.ml file generator, and that is all.

Since relative paths are more human-readable, the only drawback that I can imagine would be external tools
somehow relying on absolute path in ocamldoc error messages which does not strike me as very probable, to say the least.

Contributor

Octachron commented Dec 26, 2016

Looking at ocamldoc code, I also have a hard time to see where things could go wrong. As far as I can see, file names are not really used outside of odoc_ast.ml and odoc_sig.ml − there seems to be one use in the odoc_dot.ml file generator, and that is all.

Since relative paths are more human-readable, the only drawback that I can imagine would be external tools
somehow relying on absolute path in ocamldoc error messages which does not strike me as very probable, to say the least.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 26, 2016

Member

I found the handling of file names in ocamldoc a bit confusing in general (and I suspect it can be improved; for example, I don't think that changing the file name using a lexer directive works in all the situations it should), but clarifying this would be a long-term change.

@Octachron thanks for the review. Do is mean that you think this is good to merge?

Member

gasche commented Dec 26, 2016

I found the handling of file names in ocamldoc a bit confusing in general (and I suspect it can be improved; for example, I don't think that changing the file name using a lexer directive works in all the situations it should), but clarifying this would be a long-term change.

@Octachron thanks for the review. Do is mean that you think this is good to merge?

ocamldoc: keep using relative instead of absolute source file names
The fact that ocamldoc uses absolute file paths in its error messages
creates a usability problem for ocamlbuild users that see the path of
the files in _build instead of the path of the files in the source
project. See <ocaml/ocamlbuild#79>:

    cd /tmp
    mkdir repro; cd repro
    echo "(** hey {ol {1 bla}} *)" > a.mli
    echo "A" > doc.odocl
    ocamlbuild doc.docdir/index.html

raises the error message:

>     File "/tmp/repro/_build/a.mli", line 0, character 11:
>     Error parsing text:
>     hey {ol {1 bla}}
>                ^

and then people jump to the reported error site from their editor, and
then they are editing temporary _build files and it is a mess.

After the patch is applied, we get the following error message instead:

>     File "a.mli", line 0, character 11:
>     Error parsing text:
>     hey {ol {1 bla}}
>                ^

The code that is removed by the patch is also a mess, of undocumented
assumptions. It is careful to Sys.chdir to the argument's directory,
and to capture a long name there and Sys.chdir back. This would make
perfect sense if other parts of ocamldoc were also using 'chdir' and
one could therefore not trust relative paths. But these were the only
two places using 'chdir', so the present change actually establishes
the property that relative paths can be trusted.

I am not imaginative enough to have a sense of what can go wrong as
we turn those absolute paths into relative paths. I looked at the
blame information, this code comes from the very first ocamldoc
commits by Maxence in 2002. My guess would be that early prototypes
used 'chdir' more agressively (maybe to call the typechecker as an
external command?), making absolute paths useful.
@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Dec 26, 2016

Contributor

This looks fine to merge indeed.

Contributor

Octachron commented Dec 26, 2016

This looks fine to merge indeed.

@Octachron Octachron merged commit da93da9 into ocaml:trunk Dec 26, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #986 from gasche/ocamldoc-relative-paths
ocamldoc: keep using relative instead of absolute source file names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment