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.
(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.
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.
| (map (\(pn, cn, cid) -> | ||
| display pn ++ "=" | ||
| ++ case cn of CSubLibName sublib -> ":" ++ display sublib | ||
| _ -> "" --XXX |
fgaz
force-pushed
the
dependency-flag-component
branch
from
aa68082 to
b85ef6e
Compare
fgaz
added
the
cabal-install: v2-build system
fgaz
changed the title
fgaz
force-pushed
the
dependency-flag-component
branch
from
b85ef6e to
5b27aa9
Compare
| configConstraints :: [Dependency], -- ^Additional constraints for | ||
| -- dependencies. | ||
| configDependencies :: [(PackageName, ComponentId)], | ||
| configDependencies :: [(PackageName, ComponentName, ComponentId)], |
There was a problem hiding this comment.
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.
I'd say yes, if it's not too much of a trouble.
fgaz
force-pushed
the
dependency-flag-component
branch
from
5b27aa9 to
2eaf108
Compare
| (map (\(GivenComponent pn cn cid) -> | ||
| display pn ++ "=" | ||
| ++ case cn of CSubLibName sublib -> ":" ++ display sublib | ||
| _ -> "" --XXX |
There was a problem hiding this comment.
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.
Are you eventually planning to make internal libraries not be special cased here?
There was a problem hiding this comment.
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.
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.
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
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.
fgaz
force-pushed
the
dependency-flag-component
branch
5 times, most recently
from
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
fgaz
force-pushed
the
dependency-flag-component
branch
from
f2bd442 to
ffa7dd5
Compare
|
Closing in favor of #5526. I included this change there. |
3 participants
About time I merge something. This change should be fairly independent from the rest.
The old
--dependencycould 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=cidPlease 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