-
Notifications
You must be signed in to change notification settings - Fork 691
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 --dependency accept a component too [WIP] #5405
Conversation
<<>> quotes | ||
(disp pkgname | ||
<<>> case cname of CSubLibName sublib -> ":" <<>> disp sublib | ||
_ -> "" --XXX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(A) I'm not sure about this. Maybe I should put error "impossible non-library dependecy"
if I get something different than CLibName
or CSubLibName
. Or maybe i should add an entirely different datatype (probably not worth it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, since this is "display" code, you should still print out the non-library component in the "obvious way". If you just return an empty "" or put an error it will make it more difficult to debug if you end up with something invalid here.
Cabal/Distribution/Simple/Setup.hs
Outdated
(map (\(pn, cn, cid) -> | ||
display pn ++ "=" | ||
++ case cn of CSubLibName sublib -> ":" ++ display sublib | ||
_ -> "" --XXX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See (A)
aa68082
to
b85ef6e
Compare
b85ef6e
to
5b27aa9
Compare
Cabal/Distribution/Simple/Setup.hs
Outdated
@@ -256,7 +258,7 @@ data ConfigFlags = ConfigFlags { | |||
configStripLibs :: Flag Bool, -- ^Enable library stripping | |||
configConstraints :: [Dependency], -- ^Additional constraints for | |||
-- dependencies. | |||
configDependencies :: [(PackageName, ComponentId)], | |||
configDependencies :: [(PackageName, ComponentName, ComponentId)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now a 3-tuple. Should it be a proper datatype? something like data GivenDependency
or GivenComponent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say yes, if it's not too much of a trouble.
5b27aa9
to
2eaf108
Compare
Cabal/Distribution/Simple/Setup.hs
Outdated
(map (\(GivenComponent pn cn cid) -> | ||
display pn ++ "=" | ||
++ case cn of CSubLibName sublib -> ":" ++ display sublib | ||
_ -> "" --XXX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to update here too
cid) | ||
configDependencies = [ GivenComponent | ||
(case mb_cn of | ||
-- Special case for internal libraries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you eventually planning to make internal libraries not be special cased here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it even possible without breaking some compatibility?
configDependencies = | ||
let isMainLib CLibName = True | ||
isMainLib _ = False | ||
in filter (\(GivenComponent _ c _) -> isMainLib c) $ configDependencies flags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can make this more future robust: assuming that internal libraries switch to the new format, you can rewrite them into the old format so that old versions of Cabal can understand them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs tests. Which might be hard, because there isn't any code that actually makes use of this facility yet. Maybe if you turn this on for internal libraries (with the appropriate BC handling) that would be a good test.
a0f6d55
to
f2bd442
Compare
7.10+gold keeps OOMing (edit: not anymore \o/ ) and I don't have a windows machine to test the appveyor failure on :-/ |
The old --dependency could only do --dependency=pkg=cid, but with public sublibraries this will become insufficient. Now there is the option to also specify a component name using --dependency=pkg:component=cid
Before, a 3-tuple was used
f2bd442
to
ffa7dd5
Compare
Closing in favor of #5526. I included this change there. |
About time I merge something. This change should be fairly independent from the rest.
The old
--dependency
could only do--dependency=pkg=cid
,but with public sublibraries this will become insufficient.
Now there is the option to also specify a component name using
--dependency=pkg:component=cid
Please include the following checklist in your PR:
[ci skip]
is used to avoid triggering the build bots.Please also shortly describe how you tested your change. Bonus points for added tests!
/cc @ezyang @23Skidoo