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

fix(ocamllsp): fix in pp handling the file permissions #1153

Merged

Conversation

jboillot
Copy link
Contributor

@jboillot jboillot commented Jun 22, 2023

Hello,
In the PR that made Merlin a dependency #1070 a bug was introduced that prevents from using preprocessor programs as https://github.com/ocaml-community/cppo.
This is because the file permissions are not provided in the right base and that the output files are currently not created if they do not exist yet.
I do not know if the addition of the flag Unix.O_CREAT could cause an issue with some pp or ppx.

@jboillot jboillot force-pushed the bugfix/ocamllsp-pp-output-file-creation branch from f092f10 to d473792 Compare June 22, 2023 12:31
@rgrinberg rgrinberg requested a review from voodoos June 22, 2023 12:32
@jboillot jboillot force-pushed the bugfix/ocamllsp-pp-output-file-creation branch from d473792 to d12379f Compare June 22, 2023 12:55
@voodoos
Copy link
Collaborator

voodoos commented Jun 22, 2023

I think this needs to be fixed on Merlin's side: in the same way as for -ppx the -pp flag expect an arbitrary command, with it's arguments, so we should really handle this the same as ppxs (but we changed that behavior when refactoring).

I will do that asap.

@voodoos
Copy link
Collaborator

voodoos commented Jun 22, 2023

However the proposed change looks reasonable: having input files be read-only and allowing creating new files.

Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After having a look on Merlin's side I think things are all-right there. I added a test for preprocessing.

The issue doesn't show with ppxes because the driver is responsible for creating/writing to the file, it is not relying on redirecting stdout.

The changes look good to me.

@jboillot where you able to test the changes on your project ?

It can be done using opam pin https://github.com/jboillot/ocaml-lsp.git#bugfix/ocamllsp-pp-output-file-creation.

@rgrinberg I guess this deserves a patch release. I can take care of it if you want (and if I have the rights).

@rgrinberg
Copy link
Member

Yes, give it a try.

Where's the test though?

@voodoos
Copy link
Collaborator

voodoos commented Jun 22, 2023

The test I mentioned is on Merlin side. I am looking at how I could do the same in ocaml-lsp but it's not as simple without cram and the cli interface.

https://github.com/ocaml/merlin/pull/1630/files

@jboillot
Copy link
Contributor Author

@voodoos I was able to test the changes on a few files of a big OCaml project with cppo as the only pp (and a few other ppxes).

@voodoos
Copy link
Collaborator

voodoos commented Jun 22, 2023

I added a test that was effectively failing without the changes in this PR.

@voodoos voodoos merged commit 8131c43 into ocaml:master Jun 22, 2023
5 of 9 checks passed
@voodoos
Copy link
Collaborator

voodoos commented Jun 22, 2023

Thanks a lot @jboillot !

voodoos added a commit to voodoos/opam-repository that referenced this pull request Jun 22, 2023
CHANGES:

## Fixes

- Fix file permissions used when specifying output files of pp and ppx. (ocaml/ocaml-lsp#1153)
voodoos added a commit to voodoos/merlin that referenced this pull request Sep 20, 2023
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