-
Notifications
You must be signed in to change notification settings - Fork 433
Display a warning when dune builds via RPC #11833
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
base: main
Are you sure you want to change the base?
Conversation
48478fc
to
14c8b1b
Compare
14c8b1b
to
9c96729
Compare
Warning: A running dune (pid: 124305) instance has locked the build | ||
directory. If this is not the case, please delete "_build/.lock". | ||
The requested targets will still be built however functionality will be | ||
reduced and some command line arguments will be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of questions:
- "If this is not the case, please delete ..." is this relevant ? if the build is being forwarded to a running instance, it doesn't make much sense to me to ask the user to delete the lock file.
- "functionality will be reduced" what does this refer to?
I suggest tweaking the message and making it a bit less scary:
Warning: your build request is being forwarded to a running Dune instance (pid: 124305). Note that certain command line arguments may be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of warning the user when flags get ignored, but I would suggest that this would only happen if there are flags that will get discarded.
test/blackbox-tests/test-cases/watching/watching-eager-concurrent-build-command.t
Outdated
Show resolved
Hide resolved
8fa6b9f
to
2c531a3
Compare
Updated so that it only prints the warning if extra arguments were passed to I also added |
f39d3aa
to
7d09871
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit sad all these very verbose equal
functions need to be added but I understand the idea.
Overall I think it needs a small wording change and an additional test and then it is good to go.
bin/build_cmd.ml
Outdated
User_warning.emit | ||
[ Pp.textf | ||
"Your build request is being forwarded to a running Dune instance%s. \ | ||
Note that certain command line arguments may be ignored." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this warning is now only emitted when arguments have been ignored, I think it should state that command line arguments have in fact been ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all arguments are ignored though. The Builder
module is perhaps poorly named, as Builder.term
takes care of parsing arguments like --verbose
and --display
which still take effect when dune is behaving as an rpc client. I'll still think about how to reword this message to be more helpful though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if I understand it correctly at this point the Builder
instance is unequal to one without arguments, thus at this point we know for a fact that arguments have been ignored? Or do I misunderstand the logic of this PR?
test/blackbox-tests/test-cases/watching/watching-eager-concurrent-build-command.t
Show resolved
Hide resolved
The only [out_channel] that was ever passed in was stderr. Removing the [Out_channel] case allows us to more easily test [Log.File.t] for equality. Signed-off-by: Stephen Sherratt <[email protected]>
Signed-off-by: Stephen Sherratt <[email protected]>
Signed-off-by: Stephen Sherratt <[email protected]>
Signed-off-by: Stephen Sherratt <[email protected]>
When an instance of dune is already running, invoking `dune build` will still build the target, howwever other command line arguments are silently ignored by dune as the current RPC interface is too limited to express them. This may change over time, but currently it's unusual UX for `dune build` to behave differently depending on whether or not another instance of `dune build` is already running. To more accurately set user expectations, print a warning when dune is running with this reduced functionality. Signed-off-by: Stephen Sherratt <[email protected]>
7d09871
to
0741bbc
Compare
When an instance of dune is already running, invoking
dune build
will still build the target, howwever other command line arguments are silently ignored by dune as the current RPC interface is too limited to express them. This may change over time, but currently it's unusual UX fordune build
to behave differently depending on whether or not another instance ofdune build
is already running. To more accurately set user expectations, print a warning when dune is running with this reduced functionality.