Skip to content

Fix incomplete-record-selection warning in cabal-install #11208

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
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix incomplete-record-selection warning in cabal-install #11208

wants to merge 3 commits into from

Conversation

sgillespie
Copy link
Collaborator

Fixes #11173

I couldn't seem to find a reliable way to test this, either with the integration tests or checking the build plan. Feel free to let me know if there's a better way.

Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

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

ulysses4ever
ulysses4ever approved these changes Sep 11, 2025
View reviewed changes
Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

I had a more principled way to fix it in mind initially leveraging the fact that repoTarballPkgsWithMetadata only contains secure, so, named, repositories by construction. But I didn't find time to implement it, so maybe we should just go ahead with this patch.

A little help for future generations may be to leave a comment that all repos here are secure ones and will have a name so we're sorting Justs, so that should not have any surprises.

@sgillespie
Copy link
Collaborator Author

I had a more principled way to fix it in mind initially leveraging the fact that repoTarballPkgsWithMetadata only contains secure, so, named, repositories by construction. But I didn't find time to implement it, so maybe we should just go ahead with this patch.

A little help for future generations may be to leave a comment that all repos here are secure ones and will have a name so we're sorting Justs, so that should not have any surprises.

I suspected this might be the case, but I was trying to keep it consistent. I'm happy to iterate on this a bit more.

@sgillespie
Add a comment clarifying repoTarballPkgsWithMetadata are all SecureRepos
c25ab80 
Clarify that repoTarballPkgsWithMetadata are all SecureRepos, so this
can be easily refactored in the future.

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Commands:
#   q          Close
#   <c-c><c-c> Submit
#   <c-c><c-k> Abort
#   <m-p>      Previous Message
#   <m-n>      Next Message
#   <m-r>      Reset Message
#
# On branch fix/exe-incomplete-record-selectors
# Your branch is up to date with 'origin/fix/exe-incomplete-record-selectors'.
#
# Changes to be committed:
#	modified:   cabal-install/src/Distribution/Client/ProjectPlanning.hs
#
# Untracked files:
#	.direnv/
#
@ulysses4ever
Copy link
Collaborator

thanks @sgillespie for adding the comment! You're now welcome to add a merge label so that the bot handles it onwards. You probably want the "squash+merge" label since the second commit is logically attached to the first.

@sgillespie
Copy link
Collaborator Author

I attempted the RepoSecure by construction method by creating a new data type SecureRepo to carry around (to avoid going back and forth between Justs). However, I'm still learning the codebase and would appreciate feedback on whether this is consistent with the project conventions, or if there's a better way to approach it. Thanks for reviewing my PR!

@sgillespie sgillespie force-pushed the fix/exe-incomplete-record-selectors branch from 705ce30 to 2beb057 Compare September 22, 2025 21:17
@sgillespie
Refactor getPackageSourceHashes in cabal-install
2b0c150 
Try to leverage the fact that repoTarballPkgsWithMetadata only contains
secure Repos by construction. This allows us to avoid relying on partial
functions or maybes. Add a new data type that contains only the fields
that the RepoSecure constructor has in order to localize changes. Add
convenience functions that convert between Repo and SecureRepo.
@sgillespie sgillespie force-pushed the fix/exe-incomplete-record-selectors branch from 2beb057 to 2b0c150 Compare September 22, 2025 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geekosaur geekosaur geekosaur approved these changes

@ulysses4ever ulysses4ever ulysses4ever approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor to avoid incomplete record selection warning in cabal-install
3 participants
@sgillespie @ulysses4ever @geekosaur