cabal icon indicating copy to clipboard operation
cabal copied to clipboard

Use SetupHooks for Configure build-type

Open sheaf opened this issue 1 year ago • 8 comments

This commit implements the Configure build-type in terms of Hooks, when build-type: Hooks is available (for Cabal >= 3.13).

This moves Configure away from an implementation in terms of UserHooks, i.e. away from the Custom build-type.


Template Α: This PR modifies the Cabal API, as it changes the type of the runConfigureScript function and exposes autoconfSetupHooks from Distribution.Simple.

sheaf avatar May 03 '24 12:05 sheaf

I've added a changelog entry. I've looked through the mentions of configure build-type in the documentation but it is all agnostic to the underlying implementation, so nothing had to be updated. There already exist many tests for the configure build type, so none had to be added either.

This is a great improvement and future proofs build-type: Configure. In fact, when Hooks no longer implies legacy-fallback, neither will Configure!

alt-romes avatar May 06 '24 16:05 alt-romes

Great work. How sure are we that existing Configure type builds will keep working as before?

andreabedini avatar May 06 '24 23:05 andreabedini

Great work. How sure are we that existing Configure type builds will keep working as before?

As long as the existing testsuite can observe configure build type for working, the new implementation is working in that it also works for the same testsuite. I’m not entirely sure if you can test divergence between the two (e.g. by comparing a substitution file or environment variables or something) since we didn’t keep the old implementation.

alt-romes avatar May 07 '24 05:05 alt-romes

Great work. How sure are we that existing Configure type builds will keep working as before?

I've built stackage with a patch that contains these changes and it has successfully built all packages (including e.g. time, process etc).

sheaf avatar May 07 '24 08:05 sheaf

I've built stackage with a patch that contains these changes and it has successfully built all packages (including e.g. time, process etc).

Great :ok_hand: I wasn't entirely sure we could have expected any change in semantic.

andreabedini avatar May 07 '24 14:05 andreabedini

@andreabedini would you like to officially approve this?

alt-romes avatar May 08 '24 06:05 alt-romes

Looks good. Let me summarise:

* we leave `autoconfUserHooks` as it is

* we introduce `autoconfSetupHooks` to expose the same logic (in `runConfigureScript`) with the new API.

* cabal-install will prefer `autoconfSetupHooks` when it is available.

Do I have this right?

💯 👍

Yes. I suppose we could also remove autoconfUserHooks. The logic about which one to use is in cabal-install, and that will use whichever one is available in the Cabal library we are using.

sheaf avatar May 10 '24 08:05 sheaf

Yes. I suppose we could also remove autoconfUserHooks.

For the sake of not breaking too many things, I would suggest leaving it. That you were able to implement both APIs on top of the same core logic suggests it's not going to be burden.

andreabedini avatar May 10 '24 09:05 andreabedini