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

Reinstate 'initialBuildSteps' function (backport #9950) #9961

Merged
merged 1 commit into from
May 1, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Apr 30, 2024

This patch reinstates the 'initialBuildSteps' function, as it is used by stack in its implementation of the multi-repl feature.

A deprecation warning has been added to that function: calling it does not suffice to prepare the sources for a package, as there are other steps that one might also need to perform:

  • running pre-processors (such as alex/happy)
  • running pre-build hooks or custom logic (in build-type: Hooks or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to launch a REPL session, are advised to run setup repl --repl-multi-file=<fn> instead.

Fixes #9856


This is an automatic backport of pull request #9950 done by [Mergify](https://mergify.com).

Copy link
Contributor Author

mergify bot commented Apr 30, 2024

Cherry-pick of dd74e92 has failed:

On branch mergify/bp/3.12/pr-9950
Your branch is up to date with 'origin/3.12'.

You are currently cherry-picking commit dd74e9216.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	new file:   changelog.d/pr-9950

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   Cabal/src/Distribution/Simple/Build.hs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@Mikolaj
Copy link
Member

Mikolaj commented Apr 30, 2024

@sheaf: we expected compilation to fail but already cherry-pick failed. ;)

@sheaf sheaf force-pushed the mergify/bp/3.12/pr-9950 branch 2 times, most recently from b7e2125 to c98f00f Compare April 30, 2024 14:45
@Mikolaj Mikolaj removed the conflicts label Apr 30, 2024
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.

@Mikolaj Mikolaj requested review from ffaf1 and mpilgrem April 30, 2024 18:39
Copy link
Collaborator

@mpilgrem mpilgrem left a comment

Choose a reason for hiding this comment

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

See my suggestion as to how the Cabal (library) command might be described.

Comment on lines +941 to +942
-- launch a REPL session, are advised to run @Setup repl --repl-multi-file=<fn>@
-- instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

My only thought - and it is not a 'blocker' - is that Setup repl --repl-multi-file=<fn> could have greater consistency with the Cabal User Guide, which is written in terms of:

runhaskell Setup.hs repl --repl-multi-file=<fn>

If that is applicable, it is also applicable below.

Copy link
Member

Choose a reason for hiding this comment

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

Let me try to understand: Setup assumes the setup script is compiled (with something like ghc --make Setup.hs, I guess?) and the binary is invoked, while runhaskell Setup.hs interprets it and thus runs is? Is that it? Do both of these forms work so that it's just a user choice? If so, I'd rather avoid creating a new PR on master to change that (the original PR is already merged), unless @mpilgrem feels strongly one of the forms is misleading or bad style. But if one of these is actually wrong, please let me know and I will create the PR myself (and let's also fix here in that case).

Copy link
Collaborator

Choose a reason for hiding this comment

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

My review comment was a subtle one, and I do not feel strongly about it. I was exploring the following:

What does it mean to advise: run @Setup repl --repl-multi-file=<fn>@?

It means (as documented in the Cabal User Guide for other uses of the Cabal Haskell 'script' file named (by convention) Setup.hs) to command runhaskell Setup.hs --repl-multi-file=<fn> (or, equivalently, runghc Setup.hs --repl-multi-file=<fn>). My suggestion was to state things more plainly in the Haddock documentation and in a way consistent with the User Guide.

Copy link
Member

@Mikolaj Mikolaj May 1, 2024

Choose a reason for hiding this comment

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

I get it now, thank you. Yes, consistency is good. I'm glad that's the only problem you see in this PR, so I suggest we merge it and remember to keep docs consistent in the future (e.g., if the comment on master branch survives longer than the deprecated feature and so we'd be editing it again in the future).

@Mikolaj Mikolaj added the merge me Tell Mergify Bot to merge label May 1, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A deprecation warning has been added to that function: calling it does
not suffice to prepare the sources for a package, as there are other
steps that one might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to
launch a REPL session, are advised to run
`Setup repl --repl-multi-file=<fn>` instead.

Fixes #9856

(cherry picked from commit dd74e92)
@Mikolaj Mikolaj force-pushed the mergify/bp/3.12/pr-9950 branch from c98f00f to 14d9bed Compare May 1, 2024 19:44
@mergify mergify bot merged commit aa968f8 into 3.12 May 1, 2024
48 checks passed
@mergify mergify bot deleted the mergify/bp/3.12/pr-9950 branch May 1, 2024 22:36
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 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.

3 participants