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

Make solver able to reject build-type: Custom packages #7802

Closed
wants to merge 2 commits into from

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Nov 7, 2021


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@grayjay
Copy link
Collaborator

grayjay commented Nov 8, 2021

Could you please add more background on this flag to the commit message?

@phadej
Copy link
Collaborator Author

phadej commented Nov 8, 2021

@grayjay will do once the idea is in general accepted (along as writing docs and the changelog).

@gbaz
Copy link
Collaborator

gbaz commented Nov 18, 2021

@phadej to accept the idea in general would probably require a little motivation -- what's the plan for why we'd want this and how it would be used?

@phadej
Copy link
Collaborator Author

phadej commented Nov 18, 2021

build-type: Custom packages can run arbitrary code (technically build-type: Configure packages too). IMO it's fair to be able to ask solver to avoid them. Probably we'll need a more granular constraints, to allow some build-type: Custom packages, but this is a start.

@gbaz
Copy link
Collaborator

gbaz commented Nov 18, 2021

Is there a concrete use case for this? Are there consumers of cabal-install we can imagine wanting to turn this on by default in certain of their build systems or the like, or is the motivation more "this should exist" and somewhat speculative? Like the code looks clean to me, and the approach reasonable. I'm just not clear on if there's a general direction of work this is considered a part of or etc?

@phadej
Copy link
Collaborator Author

phadej commented Nov 19, 2021

Are there consumers

Myself. E.g. for GHCJS (or crosscompiling if it worked).

@gbaz
Copy link
Collaborator

gbaz commented Nov 19, 2021

Thanks, ghcjs was the sort of motivating use case that I was asking about! Being able to rule out plans ghcjs can't use makes perfect sense.

@phadej
Copy link
Collaborator Author

phadej commented Nov 19, 2021

GHCJS can use build-type: Custom packages (fortunately!) but things are smoother when these are avoided

@@ -344,6 +344,7 @@ convCondTree flags dr pkg os arch cinfo pn fds comp getInfo solveExes@(SolveExec
++ L.map (\e -> D.Simple (LDep dr (Ext e)) comp) (allExtensions bi) -- unconditional extension dependencies
++ L.map (\l -> D.Simple (LDep dr (Lang l)) comp) (allLanguages bi) -- unconditional language dependencies
++ L.map (\(PkgconfigDependency pkn vr) -> D.Simple (LDep dr (Pkg pkn vr)) comp) (pkgconfigDepends bi) -- unconditional pkg-config dependencies
++ [D.Simple (LDep dr (BT (buildType pkg))) comp]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line seems to add the build type constraint to every component and the branches of every conditional. Could the constraint be associated with the whole package instead, similar to UnsupportedSpecVer?

toDep (PackagePropertyInstalled) = Nothing
toDep (PackagePropertySource) = Nothing
toDep (PackagePropertyFlags _) = Nothing
toDep (PackagePropertyStanzas _) = Nothing
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this file has only whitespace changes.

@jneira
Copy link
Member

jneira commented Mar 23, 2022

@grayjay will do once the idea is in general accepted (along as writing docs and the changelog).

it seems the idea has been accepted so it could be merged if @grayjay request is addressed and docs added

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 1, 2022
@ulysses4ever ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022
@phadej
Copy link
Collaborator Author

phadej commented Dec 10, 2022

Cleaning up old PRs.

@phadej phadej closed this Dec 10, 2022
@Mikolaj Mikolaj added the PR worth reviving consider reviving this PR or remove the label with a note why not label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cabal-install: custom PR worth reviving consider reviving this PR or remove the label with a note why not
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants