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

Fix cabal install in the presence of extra-packages #9719

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Fix cabal install in the presence of extra-packages #9719

merged 1 commit into from
Mar 4, 2024

Conversation

alt-romes
Copy link
Collaborator

Extra-packages listed in a cabal project are to be fetched from hackage,
and will be in memory as 'NamedPackages' rather than resolved to
'SpecificSourcePackage'.

On install, we need to make source-dists for all the specific source
packages, and fetch other packages from hackage. Since extra-packages
are already 'NamedPackages', we simply return them along the sdistize-d
specific source packages and the hackage source packages -- they will be
correctly fetched from Hackage from install.

Previously, cabal install on a project with extra-packages would
fail because the branch of 'NamedPackage' for 'PackageSpecifier' was
simply unimplemented.

Fixes #8848

@alt-romes
Copy link
Collaborator Author

Rebased on #9706.

@alt-romes alt-romes requested a review from Kleidukos February 16, 2024 14:34
@alt-romes alt-romes force-pushed the wip/romes/8848 branch 2 times, most recently from 42af3e5 to c58174b Compare February 16, 2024 14:42
@alt-romes alt-romes changed the title Fix cabal-install in the presence of extra-packages Fix cabal install in the presence of extra-packages Feb 16, 2024
geekosaur
geekosaur approved these changes Feb 16, 2024
View reviewed changes
Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

Just some comment spelling nits.

cabal-install/src/Distribution/Client/CmdInstall.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/CmdInstall.hs Outdated Show resolved Hide resolved
@alt-romes alt-romes force-pushed the wip/romes/8848 branch 2 times, most recently from 8de7061 to 9a3c767 Compare February 19, 2024 09:40
@alt-romes alt-romes requested a review from gbaz February 19, 2024 15:29
@alt-romes alt-romes self-assigned this Feb 19, 2024
@mpickering
Copy link
Collaborator

I talked about this with Rodrigo and this fix at least does not seem worse than what is currently there.

It would be good if there are some more tests for extra-packages in the testsuite as it wasn't very clear to us what the specification for this field was supposed to be used for. The documentation mentions something about ghci, but we couldn't get ghci to work very well with extra-packages.

@andreabedini
Copy link
Collaborator

It would be good if there are some more tests for extra-packages in the testsuite as it wasn't very clear to us what the specification for this field was supposed to be used for. The documentation mentions something about ghci, but we couldn't get ghci to work very well with extra-packages.

I just extra-packages to add packages into the plan, so their presence affects the solver. I haven't thought about the implication for cabal install.

@alt-romes
Copy link
Collaborator Author

I just extra-packages to add packages into the plan, so their presence affects the solver. I haven't thought about the implication for cabal install.

We mean how extra-packages should behave in general wrt other features like build or repl..

For install the behavior should likely be what I've implemented in this patch, which is to install the specified install target (be it source, extra packages, or from hackage...)

@alt-romes alt-romes added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Mar 2, 2024
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Mar 4, 2024
@alt-romes @Mikolaj
Fix cabal-install in the presence of extra-packages
c671f0e 
Extra-packages listed in a cabal project are to be fetched from hackage,
and will be in memory as 'NamedPackages' rather than resolved to
'SpecificSourcePackage'.

On install, we need to make source-dists for all the specific source
packages, and fetch other packages from hackage. Since extra-packages
are already 'NamedPackages', we simply return them along the sdistize-d
specific source packages and the hackage source packages -- they will be
correctly fetched from Hackage from install.

Previously, cabal install <tgt> on a project with extra-packages would
fail because the branch of 'NamedPackage' for 'PackageSpecifier' was
simply unimplemented.

Fixes haskell#8848
@mergify mergify bot merged commit 8f79eb4 into haskell:master Mar 4, 2024
53 checks passed
@ffaf1
Copy link
Collaborator

ffaf1 commented Apr 9, 2024

@mergify backport 3.12

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 9, 2024 •
edited
Loading

backport 3.12

✅ Backports have been created

@mergify mergify bot mentioned this pull request Apr 9, 2024
Merged
mergify bot added a commit that referenced this pull request Apr 10, 2024
@mergify
Merge pull request #9875 from haskell/mergify/bp/3.12/pr-9719
b5ac2f7 
Fix cabal install in the presence of extra-packages (backport #9719)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffaf1 ffaf1 ffaf1 approved these changes

@geekosaur geekosaur geekosaur approved these changes

@andreabedini andreabedini Awaiting requested review from andreabedini

@Kleidukos Kleidukos Awaiting requested review from Kleidukos

@gbaz gbaz Awaiting requested review from gbaz

Assignees

@alt-romes alt-romes

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

extra-packages make cabal install fail
6 participants
@alt-romes @mpickering @andreabedini @ffaf1 @geekosaur @Kleidukos