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 #9950

Merged
merged 1 commit into from
Apr 30, 2024
Merged

Reinstate 'initialBuildSteps' function #9950

merged 1 commit into from
Apr 30, 2024

Conversation

sheaf
Copy link
Collaborator

@sheaf sheaf commented Apr 29, 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

@ffaf1
Copy link
Collaborator

ffaf1 commented Apr 29, 2024

Should we put a DEPRECATED pragma too?

@sheaf sheaf force-pushed the initialBuildSteps branch 3 times, most recently from 2dde79e to 9c9caf1 Compare April 29, 2024 09:36
@sheaf
Copy link
Collaborator Author

sheaf commented Apr 29, 2024

Should we put a DEPRECATED pragma too?

Yes, good idea. I have added a deprecation pragma, pointing users towards Setup repl --repl-multi-file=<fn>.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2024

@ulysses4ever: once this lands, the decision to backport it for inclusion in 3.12.1.0 belongs to you and the other, yet undetermined, release managers for 3.12.1.0. Me and @ffaf1 would be glad to help you make the decision. For a start, @sheaf has determined, this change is non-major-forcing, so we don't have to wait for 3.14:

it's adding a new exposed function without changing anything else so that constitutes a non-breaking change under the PVP. Non-breaking change: If only new bindings, types, classes, non-orphan instances or modules (but see below) were added to the interface, then A.B MAY remain the same but the new C MUST be greater than the old C.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2024

A link with some more details of the feature proposed as a replacement for this mechanism in the deprecation notice: #9857 (comment)

BTW, @mpilgrem, what would be a sufficient deprecation period for you? We may not state it formally in the code nor the release notes, but hinting about it in this PR is already a service to the users.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2024

Could you also include a changelog that says what this PR does? We are going to fix the release notes for 3.12.0.0 too include #9943, so it would be good to mention in 3.12.1.0 that this change has been reverted.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2024

#9943 is merged, so let's please review if the two release note entries don't contradict each other. :)

@mpilgrem
Copy link
Collaborator

mpilgrem commented Apr 29, 2024

@Mikolaj, I think the deprecation period needs only be long enough to encourage Stack users who want to use GHC > 9.8 to upgrade to Stack 2.15.7, once it is released. For people who use GHCup to manage versions of Stack, that means long enough to get Stack 2.15.7 on to GHCup's list and as 'recommended'. For people who use Stack to manage Stack, that means long enough to command stack upgrade. Plus a little time to get the message to upgrade 'out there'. So, the period could be quite short.

To recap, what Stack 2.15.5 (and earlier) does is, when somebody commands stack ghci, it includes initalBuildSteps as part of the set up. It does that by wiring that function to the replHook (and nothing more) and calling Setup.hs repl with a 'trigger' argument that triggers the use of the hook. (All of that is before my time.)

What Stack 2.15.7 does is (a) if Setup.hs is called with both repl and the 'trigger' argument, it includes initialBuildSteps (without going via the replHook mechanism) or (b) using CPP, it substitutes a copy of initialBuildSteps (more or less) if initialBuildSteps is not actually exported by Cabal.

In that regard, I see your reinstatement of initialBuildSteps has:

let compBuildDir = interpretSymbolicPathLBI lbi $ componentBuildDir lbi clbi

whereas mine has only the equivalent of:

let compBuildDir = componentBuildDir lbi clbi

so, I had better look more closely at that before Stack 2.15.7 is actually released.

EDIT: I think interpretSymbolicPathLBI is new to Cabal > 3.12.0.0, so it does not apply to my Cabal-3.12.0.0 fix.

What the Haddock documentation of initialBuildSteps now explains is that what Stack has been doing (and continues to do) is 'incomplete'. In that regard, I note that Setup.hs repl is not documented in the Cabal User Guide.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 30, 2024

@mpilgrem: thank you for your input. In that case, If nobody requests a longer deprecation period, we may get rid of initialBuildSteps in the next major version (in half a year if things get particularly speedy).

EDIT: I think interpretSymbolicPathLBI is new to Cabal > 3.12.0.0, so it does not apply to my Cabal-3.12.0.0 fix.

Ouch, well spotted. Moreover, it's introduced in a rather big PR, probably too disruptive to be backported. This means the backport of this here PR to branch 3.12 has to be tweaked (or @sheaf may choose to tweaks things already here).

I note that Setup.hs repl is not documented in the Cabal User Guide.

Noted. Unintentional, probably.

@sheaf
Copy link
Collaborator Author

sheaf commented Apr 30, 2024

I've added a changelog entry. I think this should be good to go. I will put up a backport for the 3.12 branch if we are OK to proceed with this PR.

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 @sheaf for the offer to prepare the backport, given that it will need to be different than this PR.

If nobody objects, I'm going to fast-track this PR in a couple of hours, since it fixes a problem in the just released 3.12.0.0.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 30, 2024

@mergify rebase

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
Copy link
Contributor

mergify bot commented Apr 30, 2024

rebase

✅ Branch has been successfully rebased

@Mikolaj Mikolaj added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Apr 30, 2024
@Mikolaj
Copy link
Member

Mikolaj commented Apr 30, 2024

@mergify rebase

Copy link
Contributor

mergify bot commented Apr 30, 2024

rebase

☑️ Nothing to do

  • any of:
    • #commits-behind>0 [📌 rebase requirement]
    • #commits>1 [📌 rebase requirement]
    • -linear-history [📌 rebase requirement]
  • -closed [📌 rebase requirement]
  • -conflict [📌 rebase requirement]
  • queue-position=-1 [📌 rebase requirement]

@Mikolaj Mikolaj added the merge me Tell Mergify Bot to merge label Apr 30, 2024
@mergify mergify bot merged commit a6b1eb5 into master Apr 30, 2024
51 checks passed
@mergify mergify bot deleted the initialBuildSteps branch April 30, 2024 14:30
@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 added a commit that referenced this pull request May 1, 2024
Reinstate 'initialBuildSteps' function (backport #9950)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Restore Distribution.Simple.Build.initialBuildSteps to Cabal's API
5 participants