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

Account for .buildinfo in repl when build-type: Configure #9440

Merged
merged 1 commit into from
Jan 12, 2024
Merged

Account for .buildinfo in repl when build-type: Configure #9440

merged 1 commit into from
Jan 12, 2024

Conversation

alt-romes
Copy link
Collaborator

In autoconfUserHooks we were not updating the preRepl hook to read additional build information from /package/@.buildinfo@.

Fixes #9401

@alt-romes alt-romes self-assigned this Nov 13, 2023
@alt-romes alt-romes force-pushed the wip/romes/cabal-9401 branch from c5bcfe5 to 7565c6c Compare November 13, 2023 16:54
@alt-romes
Copy link
Collaborator Author

Thanks! I'm trying to add a test, do not merge yet.

@alt-romes alt-romes mentioned this pull request Nov 13, 2023
Closed
@geekosaur
Copy link
Collaborator

You still need a second review. But if you want to block merging, mark it as a draft. (On the right sidebar)

@alt-romes alt-romes mentioned this pull request Nov 13, 2023
Closed
@Mikolaj
Copy link
Member

Mikolaj commented Nov 13, 2023

@alt-romes: FYI: https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#github-pull-request-conventions

andreabedini
andreabedini reviewed Dec 1, 2023
View reviewed changes
Copy link
Collaborator

@andreabedini andreabedini left a comment •
edited
Loading

Choose a reason for hiding this comment

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

How do we know if we have missed another one? Are these meant to be all the pre hooks? What about preTest and preBench?

@alt-romes alt-romes force-pushed the wip/romes/cabal-9401 branch 2 times, most recently from 5ca99a1 to d69d68a Compare December 18, 2023 17:03
@alt-romes
Copy link
Collaborator Author

alt-romes commented Dec 18, 2023 •
edited
Loading

@andreabedini We're indeed missing preTest and preBench.

According to my judgement this is a historical oversight and we should read information from .buildinfo for preTest and preBench too.

I'll fix the patch.

Unfortunately, I don't think there is a good systematic way of enforcing that all pre-hooks are overwritten, since the way these user hooks are built is incrementally from emptyUserHooks which still sets a lot of defaults. My hope is that by writing that we should read additional information from .buildinfo for all pre-hooks excepting for pre-conf it will become more obvious for future devs.

@alt-romes alt-romes force-pushed the wip/romes/cabal-9401 branch from d69d68a to ed10567 Compare December 18, 2023 17:25
@alt-romes
Copy link
Collaborator Author

@andreabedini done, I've updated the patch to also read additional build information for preTest and preBench. Even if we cannot enforce that all pre-hooks that are not pre-conf are updated here, at least all existing pre-hooks are now accounted for.

@alt-romes alt-romes mentioned this pull request Dec 18, 2023
Closed
@andreasabel andreasabel linked an issue Dec 20, 2023 that may be closed by this pull request
cabal repl does not respect <package>.buildinfo #4195
Closed
Kleidukos
Kleidukos previously requested changes Jan 11, 2024
View reviewed changes
Copy link
Member

@Kleidukos Kleidukos left a comment

Choose a reason for hiding this comment

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

This is missing a changelog entry, but apart from this I approve of the changes. :)

@alt-romes
Account for .buildinfo in repl when build-type: Configure
fcc78e7 
In `autoconfUserHooks` we were not updating the `preRepl` hook to read
additional build information from /package/@.buildinfo@.

Additionally updates `autoconfUserHooks` to read additional build info
information for the remaining pre-hooks that are not pre-conf.

Fixes haskell#9401
@alt-romes alt-romes force-pushed the wip/romes/cabal-9401 branch from ed10567 to fcc78e7 Compare January 11, 2024 22:27
@alt-romes alt-romes dismissed Kleidukos’s stale review January 11, 2024 22:27

Added changelog

@alt-romes
Copy link
Collaborator Author

@Kleidukos I've added the changelog, I'm just missing your final approval.

Kleidukos
Kleidukos approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@Kleidukos Kleidukos left a comment

Choose a reason for hiding this comment

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

Fantastic, thank you!

@Kleidukos
Copy link
Member

@Mergifyio backport 3.10

@Kleidukos Kleidukos merged commit ee1e6b8 into haskell:master Jan 12, 2024
50 checks passed
@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 12, 2024 •
edited
Loading

backport 3.10

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 12, 2024
@alt-romes @mergify
Account for .buildinfo in repl when build-type: Configure (#9440)
44e2b28 
In `autoconfUserHooks` we were not updating the `preRepl` hook to read
additional build information from /package/@.buildinfo@.

Additionally updates `autoconfUserHooks` to read additional build info
information for the remaining pre-hooks that are not pre-conf.

Fixes #9401

(cherry picked from commit ee1e6b8)

# Conflicts:
#	Cabal/src/Distribution/Simple.hs
@mergify mergify bot mentioned this pull request Jan 12, 2024
Merged
Kleidukos added a commit that referenced this pull request Jan 12, 2024
@mergify @alt-romes @Kleidukos
Account for .buildinfo in repl when build-type: Configure (backport #…
c805ff1 
…9440) (#9612)

* Account for .buildinfo in repl when build-type: Configure (#9440)

In `autoconfUserHooks` we were not updating the `preRepl` hook to read
additional build information from /package/@.buildinfo@.

Additionally updates `autoconfUserHooks` to read additional build info
information for the remaining pre-hooks that are not pre-conf.

Fixes #9401

(cherry picked from commit ee1e6b8)

# Conflicts:
#	Cabal/src/Distribution/Simple.hs

* Update Simple.hs

---------

Co-authored-by: Rodrigo Mesquita <rodrigo.m.mesquita@gmail.com>
Co-authored-by: Hécate Moonlight <Kleidukos@users.noreply.github.com>
@andreasabel
Copy link
Member

Thanks! I'm trying to add a test, do not merge yet.

@alt-romes @Kleidukos What happened to the test?

erikd pushed a commit to erikd/cabal that referenced this pull request Apr 22, 2024
@alt-romes @erikd
Account for .buildinfo in repl when build-type: Configure (haskell#9440)
97ac6ff 
In `autoconfUserHooks` we were not updating the `preRepl` hook to read
additional build information from /package/@.buildinfo@.

Additionally updates `autoconfUserHooks` to read additional build info
information for the remaining pre-hooks that are not pre-conf.

Fixes haskell#9401
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kleidukos Kleidukos Kleidukos approved these changes

@geekosaur geekosaur geekosaur approved these changes

@andreabedini andreabedini Awaiting requested review from andreabedini

Assignees

@alt-romes alt-romes

Labels
attention: needs-backport 3.10 attention: needs-review
Projects
None yet
Milestone
No milestone
6 participants
@alt-romes @geekosaur @Mikolaj @Kleidukos @andreasabel @andreabedini