[cabal-7825] Implement external command system#9063
[cabal-7825] Implement external command system#9063mergify[bot] merged 1 commit intohaskell:masterfrom
Conversation
|
As I have been involved in this PR I will have to recuse myself from this PR's review session. Good job putting it out there Yvan! |
|
Thank you for your contribution. It says it's a draft. Is it ready for review? |
It's ready for review (certainly not yet for merging)! |
|
As far as env vars go, I think almost any data that Cabal could provide and which could conceivably be of use to a plugin/custom command would be in the form of metadata about a package or a project, so perhaps a more principled approach to go about this would be to defer that to a separate |
geekosaur
left a comment
There was a problem hiding this comment.
I actually reviewed this before @yvan-sraka said it was ready for review 🙂 Looks good to me!
|
I would propose that the subsequent pr suggested would instead of running the command directly, do the equivalent of |
@gbaz I think you mean |
Indeed I do! |
|
I don't have a strong opinion on this but my feeling is as that I fail to see utility of the mechanism described here. I have 2.5 suggestions that could make this case stronger.
The code change is tiny, so I don't think it could harm anyone, and maybe we should be more open-minded and give it a try. Still, a broader discussion could help to converge on a better design. Maybe, an RFC could be posted on the usual channels of communications (Discord, Haskell Cafe, etc.). |
|
Thank you for your insightful feedbacks! You're correct that this PR does not really provide any technical benefits, similar to how calling Both the I plan to revise the PR to run external commands using |
Since it does add variables into the environment then it actually does add a little benefit: it saves you from setting those up manually. |
|
Did we have enough feedback for the past month? How to move this forward? |
I updated the PR with a minimalistic manual page, feedbacks welcome! |
|
This would be much more useful if some additional variables were set in the environment. At least $CABAL variable should be set I think. Does this PR support I am also a bit confused why the |
|
I confirmed that this patch also adds external commands to the |
Can you motivate the use-case? |
|
I think it was more or less agreed that which env vars to add is a subject of future work. So, it'd be reasonable to just open a ticket, I think. Perhaps, with some motivation for it, as Gershom suggests. Modification of Cabal does sound surprising and a little concerning. |
Would
It's the role of the sub-command executable to provide such flag, so
What's exactly the potential issue with that? |
|
I was looking at the cargo documentation and they suggest the most common way to implement external commands is by calling There are also some useful caveats there about how you shouldn't link against the cargo library in your custom command, perhaps things which would also be useful to be included in the documentation. Re help: I was getting confused that there was a The issue with modifying the ./Setup interface is that the PR/commit message/issue/user guide don't include this as part of the specification so it seems to be an unintended consequence of the implementation. |
I agree with that, so I will add a
Okay, so updating the user guide could be the solution? |
Fixes: #2349 and #7825
This PR introduces a simple, extensible, and functional plugin system for
cabal-install, as suggested in this comment: Plugins are standalone executables namedcabal-$COMMANDthat are available in thePATH. They can be invoked withcabal $COMMAND, and any extra command arguments are directly passed to these executables as CLI arguments. To showcase its potential, I created an example plugin,cabal-explain, which displays Haskell error codes in the terminal.Although this approach doesn't offer additional functionality over directly calling
cabal-Xat the moment, it does lay the groundwork for a more versatile and useful plugin system. A subsequent PR could incorporate aCABALenvironment variable that points to thecabal-installexecutable running the command, mirroring whatcargodoes. I welcome your feedback on these changes and would appreciate insights into anyENVvariable thatcabal-installshould set. However, IMHO, it's best to start with a minimalistic approach and avoid over-anticipating users' needs.Thank you for considering this PR. I appreciate your suggestions and comments. Special thanks to @Kleidukos, who assisted me in pair-programming this PR following a discussion in the GHC devroom at ZuriHac 2023 :)
Checklist:
Bonus points for added automated tests!
QA notes:
Lastly, I require your advice on which sections of the manual would be best to update to effectively document this feature. While developing it, I used a simple end-to-end test to validate the functionality:
Would this make a good and sufficient test to add to the
cabal-installtest suite?