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

[cabal-7825] Implement external command system #9063

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

yvan-sraka
Copy link
Collaborator

@yvan-sraka yvan-sraka commented Jun 25, 2023

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 named cabal-$COMMAND that are available in the PATH. They can be invoked with cabal $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-X at the moment, it does lay the groundwork for a more versatile and useful plugin system. A subsequent PR could incorporate a CABAL environment variable that points to the cabal-install executable running the command, mirroring what cargo does. I welcome your feedback on these changes and would appreciate insights into any ENV variable that cabal-install should 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:

echo 'echo "Hello, $1!"' > cabal-greetings
chmod +x cabal-greetings
test "$(cabal greetings World)" = "Hello, World!"

Would this make a good and sufficient test to add to the cabal-install test suite?

@Kleidukos
Copy link
Member

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!

@ulysses4ever
Copy link
Collaborator

ulysses4ever commented Jun 26, 2023

Thank you for your contribution. It says it's a draft. Is it ready for review?

@yvan-sraka
Copy link
Collaborator Author

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)!

@yvan-sraka yvan-sraka marked this pull request as ready for review June 26, 2023 06:03
@evertedsphere
Copy link

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 cabal metadata command that dumps some Cabal data structures to stdout in an easily consumable format, as Cargo does.

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

I actually reviewed this before @yvan-sraka said it was ready for review 🙂 Looks good to me!

@gbaz
Copy link
Collaborator

gbaz commented Jul 6, 2023

I would propose that the subsequent pr suggested would instead of running the command directly, do the equivalent of cabal run CMD. This should already setup an environment with a lot of information in scope.

@fgaz
Copy link
Member

fgaz commented Jul 6, 2023

cabal run CMD

@gbaz I think you mean cabal exec

@gbaz
Copy link
Collaborator

gbaz commented Jul 7, 2023

I think you mean cabal exec

Indeed I do!

@ulysses4ever
Copy link
Collaborator

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.

  1. It'd be interesting to know if cargo had any notable success with this mechanism. Judging by the link in OP, cargo exposes only one env var, which makes me think that the demand for this mechanism is not high.

  2. I'd be interesting to hear which env vars cabal could, in principle, share with the ad hoc subcommands, and how those vars could be used. Ultimately, it'd be cool to hear what kind of realistic tools could benefit from such architecture. The toy example implemented to showcase the mechanism is helpful to understand how it works but doesn't convince that any good can come out of it.

  3. As requested above, comparison with cabal exec would be nice to see.

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.).

@yvan-sraka
Copy link
Collaborator Author

yvan-sraka commented Jul 10, 2023

Thank you for your insightful feedbacks! You're correct that this PR does not really provide any technical benefits, similar to how calling cargo foobar or git barfoo does not provide value over calling directly cargo-foobar or git-barfoo. However, in my opinion, this is more of a discussion about user experience. This minor difference in syntax can significantly influence how users perceive these commands. Even though they are technically equivalent, the former syntax makes external commands feel more like an integral part of the main tool, even when they're third-party. The idea is to create a more ergonomic and consistent way for users to extend cabal's capabilities.

Both the cargo and git ecosystems seem to display wide popularity and user adoption of this feature. This is evidenced by the vast number of binary crates following this pattern and the several practical git extensions. I can tell that I personally use many of these tools in my everyday workflow!

I plan to revise the PR to run external commands using cabal exec. I initially overlooked this, but it seems like the right approach. Furthermore, I'm definitely open to a broader discussion, and I'd be more than happy to draft an RFC if that seems necessary. Publicizing the PR certainly couldn't hurt! :)

@Kleidukos
Copy link
Member

https://discourse.haskell.org/t/an-external-command-system-for-cabal-what-would-you-do-with-it/7114/4

@eyeinsky
Copy link

this PR does not really provide any technical benefits

Since it does add variables into the environment then it actually does add a little benefit: it saves you from setting those up manually.

@Mikolaj
Copy link
Member

Mikolaj commented Sep 4, 2023

Did we have enough feedback for the past month? How to move this forward?

@yvan-sraka
Copy link
Collaborator Author

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!

doc/external-commands.rst Outdated Show resolved Hide resolved
@andreabedini andreabedini added the squash+merge me Tell Mergify Bot to squash-merge label Oct 10, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Oct 12, 2023
@yvan-sraka yvan-sraka removed the squash+merge me Tell Mergify Bot to squash-merge label Oct 28, 2023
@Kleidukos Kleidukos added merge me Tell Mergify Bot to merge and removed merge me Tell Mergify Bot to merge labels Oct 31, 2023
@yvan-sraka yvan-sraka added merge me Tell Mergify Bot to merge and removed merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Nov 1, 2023
@Kleidukos Kleidukos added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Nov 2, 2023
@mergify mergify bot merged commit f6565d2 into haskell:master Nov 2, 2023
45 checks passed
@yvan-sraka yvan-sraka deleted the fix-7825 branch November 2, 2023 14:09
@mpickering
Copy link
Collaborator

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 --help for subcommands?

I am also a bit confused why the Cabal library has been modified at all. Does this mean that ./Setup.hs interface also supports external commands?

@mpickering
Copy link
Collaborator

I confirmed that this patch also adds external commands to the ./Setup.hs interface which doesn't seem to be expected.

@gbaz
Copy link
Collaborator

gbaz commented Nov 2, 2023

At least $CABAL variable should be set I think.

Can you motivate the use-case?

@ulysses4ever
Copy link
Collaborator

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.

@yvan-sraka
Copy link
Collaborator Author

This would be much more useful if some additional variables were set in the environment. At least $CABAL variable should be set I think.

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.

Would $CABAL be the path to the cabal binary spawning the command?

Does this PR support --help for subcommands?

It's the role of the sub-command executable to provide such flag, so cabal foobar --help could be expected to work, while cabal --help foobar will not, is that your concern?

I am also a bit confused why the Cabal library has been modified at all. Does this mean that ./Setup.hs interface also supports external commands?

I confirmed that this patch also adds external commands to the ./Setup.hs interface which doesn't seem to be expected.

Modification of Cabal does sound surprising and a little concerning.

What's exactly the potential issue with that?

@mpickering
Copy link
Collaborator

I was looking at the cargo documentation and they suggest the most common way to implement external commands is by calling $CARGO from within the wrapper script. I can imagine this is true as otherwise you have no (robust) way to interact with the build tool and hence inspect/invoke anything which the build tool does.

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 cabal help command, but there's not, so ignore me there.

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.

@yvan-sraka
Copy link
Collaborator Author

yvan-sraka commented Nov 3, 2023

I was looking at the cargo documentation and they suggest the most common way to implement external commands is by calling $CARGO from within the wrapper script. I can imagine this is true as otherwise you have no (robust) way to interact with the build tool and hence inspect/invoke anything which the build tool does.

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.

I agree with that, so I will add a $CABAL env variable in a follow-up PR :)

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.

Okay, so updating the user guide could be the solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow cabal to execute user-defined subcommands