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

Solver: Support dependencies on sub-libraries (issue #6039) #6812

Closed
wants to merge 5 commits into from

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented May 18, 2020

Rebased #6047

@phadej
Copy link
Collaborator Author

phadej commented May 18, 2020

@fgaz what else is needed to be done to make PackageTests/MultipleLibraries/Successful/cabal.test.hs pass on GHC-8.6.5?

or is this a "handling of installing packages" which @grayjay mentions in #6047 (comment)

If so, feel free to merge as soon as CI is green.

@phadej phadej added this to the 3.4.0.0 milestone May 18, 2020
@phadej
Copy link
Collaborator Author

phadej commented May 18, 2020

Also remember to write a chengelog entry (See changelog.d directory).

@phadej
Copy link
Collaborator Author

phadej commented May 18, 2020

@grayjay do you have 2FA enabled on GitHub. Looks like you are not a collaborator of Cabal anymore (so I cannot assign issues to you). I'd like to add you back.

@fgaz
Copy link
Member

fgaz commented May 19, 2020

@grayjay I see you didn't pass --allow-depending-on-private-libs to Cabal to override the visibility checks.

As per #6047 (comment) we should either do that or fail early, otherwise this still fails late in the build on <8.8

@phadej
Copy link
Collaborator Author

phadej commented May 19, 2020

Looks like there's a conflict. I'll rebase again shortly.

grayjay added 5 commits May 20, 2020 13:36
This commit tracks dependencies on sub-libraries by extending the functionality
for tracking executables that was added in
e86f838.

It also starts adding support for library visibility, though it currently only
works for source packages.  There is a TODO for handling installed packages.

This commit handles visibility similarly to the way that the buildable field is
handled currently.  It only checks whether a component is made private by the
current environment and flag constraints at the start of dependency solving.
This means that the solver can treat a component as visible when the visibility
is controlled by an automatic flag, and the build can fail later, depending on
the value that is chosen for the flag.

Fixes haskell#6038.
This commit also refactors the Dependencies type so that it can represent any
combination of dependencies, buildability, and visibility.
@phadej
Copy link
Collaborator Author

phadej commented May 20, 2020

rebased.

@fgaz
Copy link
Member

fgaz commented May 22, 2020

And here's the line that should be changed:

-- TODO set to true when the solver can prevent private-library-deps by itself
-- (issue #6039)
configAllowDependingOnPrivateLibs = mempty

Ideally we'd set it to true only for GHC <8.8, so that we'd still have Cabal's sanity checks for newer GHCs

@fgaz
Copy link
Member

fgaz commented May 22, 2020

This should work. Please take a look @phadej

@fgaz
Copy link
Member

fgaz commented May 22, 2020

@phadej I just force-with-lease pushed and only later realized that this is your branch, sorry

@phadej
Copy link
Collaborator Author

phadej commented May 22, 2020 via email

@phadej
Copy link
Collaborator Author

phadej commented May 22, 2020

I repushed my version. Sorry for the lsot commit @fgaz, but with over hunded people with write access, I cannot permit pushing to my repository.

It's disastrous, not convenient. This is why I opened a separate PR in the first place.

Please DO NOT PUSH INTO OTHERS BRANCHES BEFORE ASKING FOR PERMISSION.

@phadej
Copy link
Collaborator Author

phadej commented May 22, 2020

And allow edit by maintainers setting doesn't seem to be preserved. :(

@fgaz
Copy link
Member

fgaz commented May 22, 2020

Sorry for the lsot commit

Nothing lost :) I opened another pr with my change: #6836

over hunded people

Wow, a hundred already? That's a lot

Sorry for pushing on your branch, I saw the "Add more commits by pushing to the pr-6047 branch on phadej/cabal." message and assumed it was ok to do that.

I can't help but notice that this is yet another flaw in the PR model. Multiple people cannot iterate on a patch without opening a million prs.

@phadej phadej closed this May 22, 2020
@phadej phadej deleted the pr-6047 branch May 22, 2020 18:51
@phadej
Copy link
Collaborator Author

phadej commented May 22, 2020

collaboration needs communication, pushing into branches "owned" by others without an agreement is not polite.

@grayjay
Copy link
Collaborator

grayjay commented May 26, 2020

@grayjay do you have 2FA enabled on GitHub. Looks like you are not a collaborator of Cabal anymore (so I cannot assign issues to you). I'd like to add you back.

@phadej I enabled 2FA and then tried to accept your invitation, but it had expired. Could you please resend it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants