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.

Sign up for GitHub

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

Jump to bottom

Add pre and post build hooks #9899

Closed
wants to merge 1 commit into from
Closed

Add pre and post build hooks #9899

wants to merge 1 commit into from

Conversation

erikd
Copy link
Member

@erikd erikd commented Apr 17, 2024 •
edited
Loading

Run a program (named "preBuildHook") before doing a package build and another program (named "postBuildHook") after the package is built.

These two programs simply need to be on the users $PATH variable and are completely generic.

Co-authored: Moritz Angermann [email protected]

Related to: #9892

Please read Github PR Conventions and then fill in one of these two templates.

Template Α: This PR modifies cabal behaviour

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

Template Β: This PR does not modify cabal behaviour (documentation, tests, refactoring, etc.)

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

@erikd erikd marked this pull request as draft April 17, 2024 04:34
@erikd erikd self-assigned this Apr 17, 2024
@erikd erikd force-pushed the erikd/pre-post-build-hooks branch 3 times, most recently from f40888c to 56349de Compare April 17, 2024 04:52
@erikd erikd mentioned this pull request Apr 17, 2024
Open
@angerman angerman requested a review from hasufell April 17, 2024 05:38
@erikd erikd force-pushed the erikd/pre-post-build-hooks branch 2 times, most recently from c5cd321 to ed08c4e Compare April 17, 2024 06:08
hasufell
hasufell reviewed Apr 17, 2024
View reviewed changes
Copy link
Member

@hasufell hasufell left a comment

Choose a reason for hiding this comment

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

Needs documentation (readthedocs)

cabal-install/src/Distribution/Client/ProjectBuilding/UnpackedPackage.hs Outdated
Comment on lines 693 to 696
when (code /= ExitSuccess) $ do
runBuild
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it's not really a hook in the original sense anymore, because it causes an internal cabal phase to be skipped entirely. Why?

The way e.g. git hooks are designed is they just augment existing phases. An error code of e.g. a pre-commit hook, will make the commit as a whole fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could be more explicit in using the exit code to signal whether or not we want the subsequent action to happen or be skipped. You may want the hook to allow skipping under certain conditions.

cabal-install/src/Distribution/Client/ProjectBuilding/UnpackedPackage.hs
`catchIO` (\_ -> return (ExitFailure 10))
when (code /= ExitSuccess) $ do
runBuild
-- not sure, if we want to care about a failed postBuildHook?
Copy link
Member

Choose a reason for hiding this comment

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

I argue we should. The author of the script can always hide errors of whatever commands they're executing inside the hook.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would you propose?

cabal-install/src/Distribution/Client/ProjectBuilding/UnpackedPackage.hs Outdated
Comment on lines 685 to 699
code <-
rawSystemExitCode
verbosity
(Just srcdir)
(hookDir </> "preBuildHook")
[ (unUnitId $ installedUnitId rpkg)
, (getSymbolicPath srcdir)
, (getSymbolicPath builddir)
]
Copy link
Member

Choose a reason for hiding this comment

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

This needs much more discussion.

  1. relying on PATH to find hooks is very improper imo (I see you already changed it)
    • this should be in something like ~/.cabal/hooks and the user should be able to configure the location via the cabal config
  2. just like git, we should only execute hooks if they're marked as executable (windows is a special case, see how we did it in stack as an example)
  3. we need a discussion what information we want to expose (e.g. env vars that are available to all hooks, like CABAL_VERSION or GHC_VERSION?)

Copy link
Member Author

Choose a reason for hiding this comment

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

The PATH thing has been replaced with looking up ~/.cabal/hooks/.

Copy link
Member Author

Choose a reason for hiding this comment

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

As per the discussion ticket, this now uses a project local cabalHooks directory.

cabal-install/src/Distribution/Client/ProjectBuilding/UnpackedPackage.hs Outdated
, (getSymbolicPath srcdir)
, (getSymbolicPath builddir)
]
`catchIO` (\_ -> return (ExitFailure 10))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced we should overwrite the exit code of the script?

Copy link
Collaborator

Choose a reason for hiding this comment

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

there is no exit code. This is file not found mapped to ExitFailure 10. Yes 10 is just a magic constant.

cabal-install/src/Distribution/Client/ProjectBuilding/UnpackedPackage.hs Outdated Show resolved Hide resolved
@erikd erikd force-pushed the erikd/pre-post-build-hooks branch 5 times, most recently from 5951fc0 to 7f4ea63 Compare April 24, 2024 01:20
angerman added a commit to input-output-hk/haskell-nix-example that referenced this pull request May 7, 2024
@angerman
bump cabal-install to point to haskell/cabal#9899
26b414c
@erikd erikd force-pushed the erikd/pre-post-build-hooks branch 2 times, most recently from 6db998e to c99f431 Compare May 7, 2024 06:43
@erikd
Copy link
Member Author

erikd commented May 7, 2024 •
edited
Loading

Needs documentation (readthedocs)

Preliminary documentation added.

@erikd erikd force-pushed the erikd/pre-post-build-hooks branch 3 times, most recently from bb9c361 to ffa9b5e Compare May 7, 2024 07:14
@erikd erikd marked this pull request as ready for review May 8, 2024 02:10
@erikd
Copy link
Member Author

erikd commented May 8, 2024

I am not in a huge hurry to merge this. I am especially interested in hearing people's opinions on the "Security Considerations" section in the documentation.

@erikd erikd marked this pull request as draft May 9, 2024 05:31
@erikd erikd force-pushed the erikd/pre-post-build-hooks branch from bc04f49 to 98b6d7f Compare May 9, 2024 05:45
@erikd
Copy link
Member Author

erikd commented May 9, 2024 •
edited
Loading

Currently the return codes of the two hooks are just ignored, but it might be useful to pass the exit code of the pre-build hook to the post-build hook.

The return code of the pre-build hook is not passed as a parameter to the post-build hook.

@erikd erikd force-pushed the erikd/pre-post-build-hooks branch from 33aabe0 to b5c7050 Compare May 10, 2024 03:17
@erikd erikd marked this pull request as ready for review May 10, 2024 03:54
@erikd erikd force-pushed the erikd/pre-post-build-hooks branch from b5c7050 to 466e484 Compare May 12, 2024 21:25
@erikd erikd force-pushed the erikd/pre-post-build-hooks branch from 466e484 to d16ecc3 Compare June 17, 2024 02:09
theGhostJW
theGhostJW reviewed Jul 13, 2024
View reviewed changes
doc/build-hooks.rst Outdated Show resolved Hide resolved
@erikd
Add pre and post build hooks
16bd68d 
Run a program (named "preBuildHook") before doing a package build and
another program (named "postBuildHook") after the package is built.
The exit code from the pre-build hook is passed to the post-build
hook.

These programs are project local and need to be in the `cabalHooks`
directory which is in the same directory as the `cabal.project` file.

Co-authored: Moritz Angermann <[email protected]>
@erikd
Copy link
Member Author

erikd commented Sep 18, 2024

Rebased against master and doc typo fixed.

@erikd
Copy link
Member Author

erikd commented Sep 25, 2024

Pretty sure it would not be a good idea to add this.

Having the hooks global to the user is a insufficiently flexible (make defining different hook functionality for different projects inconvenient.)

Having hooks project local (ie store in git) is dangerous from a security POV. Anyone that gets access to the repo can modify the hooks and insert malicious code. I do not see any solution to this. A single organization may want to implement something like this, but I cannot see this as a good idea for every instance of cabal.

@erikd erikd closed this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theGhostJW theGhostJW theGhostJW left review comments

@hasufell hasufell hasufell left review comments

@andreabedini andreabedini andreabedini left review comments

@angerman angerman angerman left review comments

Assignees

@erikd erikd

Labels
attention: needs-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@erikd @angerman @andreabedini @hasufell @theGhostJW @geekosaur