Skip to content

Conversation

@ElectreAAS
Copy link
Collaborator

@ElectreAAS ElectreAAS commented Jul 22, 2025

This PR is a second step towards implementing #11958, and is the 'big sister' of #12010.
Like its sister, this PR contains stolen code from Steve's #11900. 'Stolen' code has been merged on main

It is currently in draft mode It is now ready for review! The mechanism for promotion is now sound :)

@ElectreAAS ElectreAAS requested a review from shonfeder July 22, 2025 14:55
@shonfeder shonfeder requested a review from gridbugs July 22, 2025 15:12
@shonfeder
Copy link
Member

Hi! Could you provide some context by explaining what you find sketchy about the enabling?

@shonfeder shonfeder self-requested a review July 24, 2025 17:20
@ElectreAAS ElectreAAS force-pushed the format-with-watch branch 3 times, most recently from 2ca0ac7 to 9f8f770 Compare July 31, 2025 09:39
@ElectreAAS ElectreAAS marked this pull request as ready for review August 21, 2025 15:02
let f _ promote =
let server = Fdecl.get t in
let outcome = Fiber.Ivar.create () in
let target = server.parse_build_arg "(alias_rec fmt)" in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sold on the idea of having the target be determined by a string parse like this, isn't there a cleaner way?

Copy link
Collaborator

@Alizter Alizter Aug 21, 2025

Choose a reason for hiding this comment

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

We don't have access to any of the information of the build targets here since interpretation comes at a later point. If you are uncomfortable by the string, you could use

      let target =
        Sexp.(List [ Atom "alias_rec"; Atom "fmt" ])
        |> Sexp.to_string
        |> server.parse_build_arg
      in

but ultimately it's the same thing.

Copy link
Member

Choose a reason for hiding this comment

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

Not sold on the idea of having the target be determined by a string parse like this, isn't there a cleaner way?

The cleaner way is to introduce a new build request that offers a stable type for targets.

You are right to be suspicious of this since the way the server is parsing this string depends is dependent on the dune lang. This makes RPC more fragile than needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dune_lang.Dep_conf.Alias_rec (Dune_lang.String_with_vars.make_text Loc.none "fmt") is how it's now done. If there is another way about doing this then I don't know what it is

@ElectreAAS ElectreAAS force-pushed the format-with-watch branch 4 times, most recently from 84568f6 to 77a9fba Compare August 26, 2025 16:23
stdune
unix)
(synopsis "Dune's rpc server + a usable client"))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pet peeve, but this should be sorted. I added dune_lang as I pull it up as dependency in server.ml

@ElectreAAS ElectreAAS requested a review from rgrinberg September 2, 2025 14:40
@shonfeder
Copy link
Member

shonfeder commented Sep 4, 2025

@ElectreAAS Can this be merged now?

@ElectreAAS
Copy link
Collaborator Author

ElectreAAS commented Sep 5, 2025

@ElectreAAS Can this be merged now?

No! There is a problem with the cache, and the --preview situation isn't yet under control

Now, yes it is!

@ElectreAAS ElectreAAS force-pushed the format-with-watch branch 2 times, most recently from a79bdb7 to f4e74ad Compare September 8, 2025 12:46
Signed-off-by: Ambre Austen Suhamy <[email protected]>
Signed-off-by: Ambre Austen Suhamy <[email protected]>
Signed-off-by: Ambre Austen Suhamy <[email protected]>
Signed-off-by: Ambre Austen Suhamy <[email protected]>
Signed-off-by: Ambre Austen Suhamy <[email protected]>
Signed-off-by: Ambre Austen Suhamy <[email protected]>
@ElectreAAS ElectreAAS merged commit 27e91d1 into ocaml:main Sep 8, 2025
26 checks passed
@ElectreAAS ElectreAAS deleted the format-with-watch branch September 8, 2025 14:37
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.

4 participants