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

Add initialBuildSteps removal notice #9943

Merged
merged 2 commits into from Apr 29, 2024
Merged

Conversation

ffaf1
Copy link
Collaborator

@ffaf1 ffaf1 commented Apr 27, 2024

And suggestion on what to use (preBuildComponent) instead.

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

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Thank you very much.

Could we also mention the PR that removed this? (#9474)

@ffaf1
Copy link
Collaborator Author

ffaf1 commented Apr 29, 2024

Sure and done

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2024

CI is dead, but since this is only a documentation fix and an urgent one, unless @sheaf would like to suggest changes, I'd propose to expedite it. If nobody objects, I'd merge manually in a couple of hours.

@sheaf
Copy link
Collaborator

sheaf commented Apr 29, 2024

I don't think we want to suggest preBuildComponent, because that still does not handle all preparation of sources such as running preprocessors or custom logic. Perhaps we can point to ./Setup repl --repl-multi-file=<fn> instead?

@sheaf
Copy link
Collaborator

sheaf commented Apr 29, 2024

Thanks for putting up this PR BTW.

@ffaf1
Copy link
Collaborator Author

ffaf1 commented Apr 29, 2024

I don't think we want to suggest preBuildComponent, because that still does not handle all preparation of sources such as running preprocessors or custom logic. Perhaps we can point to ./Setup repl --repl-multi-file= instead?

What would a library user write, instead of initialBuildSteps?

@sheaf
Copy link
Collaborator

sheaf commented Apr 29, 2024

I don't think we want to suggest preBuildComponent, because that still does not handle all preparation of sources such as running preprocessors or custom logic. Perhaps we can point to ./Setup repl --repl-multi-file= instead?

What would a library user write, instead of initialBuildSteps?

My question is: what is this library user trying to do? I think the only reason one would be calling this function is because one wants Cabal to prepare source files for a package; however, initialBuildSteps only handles things like the paths module and not anything else. I suppose it is OK for a consumer to re-implement all the other logic themselves (running pre-processors, running pre-build hooks, etc), but that seems like duplication of effort that will just lead to more bugs.

So my answer would still be to call ./Setup repl --repl-multi-file, even when using a library interface. Nothing else is correct in the presence of a custom Setup. If one assumes the Setup is not custom, then one can instead directly call repl_setupHooks with the correct pre-build hooks of the package (if there are any).

And suggestion on what to use (`preBuildComponent`) instead.
@ffaf1
Copy link
Collaborator Author

ffaf1 commented Apr 29, 2024

Thanks @sheaf. I added your reasoning (with slight rewording) to the release notes. If you are fine with it, it can get merged.

Co-authored-by: sheaf <sam.derbyshire@gmail.com>
@ffaf1 ffaf1 added squash+merge me Tell Mergify Bot to squash-merge and removed merge me Tell Mergify Bot to merge labels Apr 29, 2024
@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2024

As advertised here and in the channel, I'm merging before the delay is up and ignoring CI. Thank you all!

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2024

@mergify rebase

Copy link
Contributor

mergify bot commented Apr 29, 2024

rebase

✅ Nothing to do for rebase action

@Mikolaj Mikolaj merged commit c80ee75 into haskell:master Apr 29, 2024
13 of 43 checks passed
@Mikolaj
Copy link
Member

Mikolaj commented Apr 30, 2024

@mergify backport 3.12

Copy link
Contributor

mergify bot commented Apr 30, 2024

backport 3.12

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Apr 30, 2024
* Add `initialBuildSteps` removal notice

And suggestion on what to use (`preBuildComponent`) instead.

* Update release-notes/Cabal-3.12.0.0.md

Co-authored-by: sheaf <sam.derbyshire@gmail.com>
(cherry picked from commit c80ee75)
mergify bot added a commit that referenced this pull request Apr 30, 2024
Add `initialBuildSteps` removal notice (backport #9943)
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants