Finish off external commands feature #9412

mpickering · 2023-11-06T16:57:40Z

Finish off the external commands feature

* Remove 'CommandDelegate' in favour of abstracting the fallback in
  'commandsRun', there is a new variant 'commdandRunWithFallback' which
  takes a continuation
  - This restores the modularity between the `Cabal` library and
    `cabal-install` as now `Cabal` doesn't need to know anything about
    the external command interface.
  - Fixes #9403
* Set the $CABAL environment variable to the current executable path
  - This allows external commands to be implemented by calling $CABAL,
    which is strongly preferred to linking against the Cabal library as
    there is no easy way to guantee your tool and `cabal-install` link
    against the same `Cabal` library.
  - Fixes #9402
* Pass the name of the argument
  - This allows external commands to be implemented as symlinks to an
    executable, and multiple commands can be interpreted by the same
    executable.
  - Fixes #9405
* `cabal help <cmd>` is interpreted as `cabal-<cmd> --help` for external
  commands.
  - This allows the `help` command to also work for external
  commands and hence they are better integrated into cabal-install.
  - Fixes #9404

The tests are updated to test all these additions.

These features bring the external command interface up to par with the
cargo external command interface.

yvan-sraka

LGTM! Great to have this feature rewritten in better shape, thanks a lot for that and all the corner cases you spotted and fixed :)

I'm new to Cabal contributors and not sure to understand the point of adding new LICENSE files?

geekosaur · 2023-11-07T10:35:07Z

They're added for the tests, which are implemented as full Cabal packages and therefore require license files.

mpickering · 2023-11-06T17:00:49Z

cabal-install/src/Distribution/Client/Main.hs

@@ -355,6 +355,28 @@ mainWorker args = do
            warnIfAssertionsAreEnabled
            action globalFlags
  where
+    delegateToExternal :: [Command Action]


The logic for finding and calling subcommands is now contained solely here (inside cabal-install) rather than leaking into the Cabal library.

mpickering · 2023-11-06T17:01:23Z

Cabal/src/Distribution/Simple/Command.hs

+  -> IO (CommandParse action)
+defaultCommandFallback commands' name _cmdArgs = pure $ badCommand commands' name
+
+commandsRunWithFallback


commandRun is parameterised by a continuation which is used to handle the fallback case in different ways for Cabal and cabal-install.

mpickering · 2023-11-07T13:55:05Z

cabal-install/src/Distribution/Client/Main.hs

+                  -> [String]
+                  -> IO (CommandParse Action)
+    delegateToExternal commands' name cmdArgs = do
+      mCommand <- findProgramOnSearchPath normal defaultProgramSearchPath ("cabal-" <> name)


I thought this would fix the test failure on windows but it still seems to persist.

mpickering · 2023-11-09T15:18:18Z

I have fixed CI now, so this patch is ready to merge.

Mikolaj · 2023-11-09T15:25:50Z

I've given @yvan-sraka the Triage authority which, once accepted, will make the review binding for github.

@mpickering: feel free to set a merge label, unless you'd like more reviews, in which case, please set the appropriate label, too.

Mikolaj · 2023-11-23T11:59:01Z

It turns out Write permission is needed for reviews to be counted for the 2-review threshold, so I've given this high level of authority to @yvan-sraka. I hope in 2 days this gets auto-merged. (Let me rebase via mergify just in case; edit: especially that CI mandatory tests settings have changed, so this is stuck.)

Mikolaj · 2023-11-23T12:00:08Z

@mergify rebase

mergify · 2023-11-23T12:00:37Z

rebase

✅ Branch has been successfully rebased

This adds 4 tests which test the new external commands feature: * ExternalCommand - Tests the expected usage of external command invoked via cabal-install * ExternalCommandSetup - Tests that the ./Setup interface does not support external commands (haskell#9403) * ExternalCommandEnv - Tests that environment variables are set and preserved appropiately (haskell#9402) * ExternalCommandHelp - Test that `cabal help <cmd>` is interpreted appropiately (haskell#9404)

* Remove 'CommandDelegate' in favour of abstracting the fallback in 'commandsRun', there is a new variant 'commdandRunWithFallback' which takes a continuation - This restores the modularity between the `Cabal` library and `cabal-install` as now `Cabal` doesn't need to know anything about the external command interface. - Fixes haskell#9403 * Set the $CABAL environment variable to the current executable path - This allows external commands to be implemented by calling $CABAL, which is strongly preferred to linking against the Cabal library as there is no easy way to guantee your tool and `cabal-install` link against the same `Cabal` library. - Fixes haskell#9402 * Pass the name of the argument - This allows external commands to be implemented as symlinks to an executable, and multiple commands can be interpreted by the same executable. - Fixes haskell#9405 * `cabal help <cmd>` is interpreted as `cabal-<cmd> --help` for external commands. - This allows the `help` command to also work for external commands and hence they are better integrated into cabal-install. - Fixes haskell#9404 The tests are updated to test all these additions. These features bring the external command interface up to par with the cargo external command interface.

Mikolaj · 2023-11-24T15:24:04Z

I took the liberty of making up for the admin delays by setting the merge_delay_passed label manually. The PR is ready and reviewed since last week.

mpickering changed the title ~~Wip/external commands fixes~~ Finish off external commands feature Nov 6, 2023

mpickering force-pushed the wip/external-commands-fixes branch from c3b8109 to 02605e9 Compare November 6, 2023 16:59

mpickering mentioned this pull request Nov 6, 2023

External commands: Add tests for #9402 #9403 #9404 #9406

Closed

geekosaur approved these changes Nov 6, 2023

View reviewed changes

yvan-sraka self-requested a review November 7, 2023 07:50

yvan-sraka approved these changes Nov 7, 2023

View reviewed changes

yvan-sraka mentioned this pull request Nov 7, 2023

[cabal-7825] Add $CABAL env variable for external commands #9393

Closed

4 tasks

BinderDavid added the cabal-install: cmd/external-cmd label Nov 7, 2023

mpickering force-pushed the wip/external-commands-fixes branch from 02605e9 to 56a8cb6 Compare November 7, 2023 12:34

mpickering commented Nov 7, 2023

View reviewed changes

mpickering force-pushed the wip/external-commands-fixes branch from 2a803aa to ec076de Compare November 9, 2023 13:20

mpickering added the merge me Tell Mergify Bot to merge label Nov 16, 2023

yvan-sraka mentioned this pull request Nov 23, 2023

Improving Cabal Solver Output for Better Readability and Usability #8939

Open

Mikolaj force-pushed the wip/external-commands-fixes branch from ec076de to 1c3bd64 Compare November 23, 2023 12:00

Mikolaj added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Nov 24, 2023

mpickering added 2 commits November 24, 2023 15:22

Mikolaj force-pushed the wip/external-commands-fixes branch from 1c3bd64 to d8ebb81 Compare November 24, 2023 15:22

mergify bot merged commit 4f53a2f into haskell:master Nov 24, 2023
30 of 36 checks passed

MangoIV mentioned this pull request Aug 25, 2024

external commands need to explicitly handle their own name being passed to the executable #10275

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish off external commands feature #9412

Finish off external commands feature #9412

mpickering commented Nov 6, 2023

yvan-sraka left a comment

geekosaur commented Nov 7, 2023

mpickering Nov 6, 2023

mpickering Nov 6, 2023

mpickering Nov 7, 2023

mpickering commented Nov 9, 2023

Mikolaj commented Nov 9, 2023

Mikolaj commented Nov 23, 2023 •

edited

Loading

Mikolaj commented Nov 23, 2023

mergify bot commented Nov 23, 2023

Mikolaj commented Nov 24, 2023

Finish off external commands feature #9412

Finish off external commands feature #9412

Conversation

mpickering commented Nov 6, 2023

yvan-sraka left a comment

Choose a reason for hiding this comment

geekosaur commented Nov 7, 2023

mpickering Nov 6, 2023

Choose a reason for hiding this comment

mpickering Nov 6, 2023

Choose a reason for hiding this comment

mpickering Nov 7, 2023

Choose a reason for hiding this comment

mpickering commented Nov 9, 2023

Mikolaj commented Nov 9, 2023

Mikolaj commented Nov 23, 2023 • edited Loading

Mikolaj commented Nov 23, 2023

mergify bot commented Nov 23, 2023

✅ Branch has been successfully rebased

Mikolaj commented Nov 24, 2023

Mikolaj commented Nov 23, 2023 •

edited

Loading