Target package names #4734
Target package names #4734
Conversation
Consolodate the three copies of it.
|
@dcoutts, thanks for your PR! By analyzing the history of the files in this pull request, we identified @grayjay, @RyanGlScott and @hvr to be potential reviewers. |
|
|
||
| None errs | ||
| | TargetStringFileStatus1 str _ <- targetStrStatus | ||
| , validPackageName str -> Right (TargetPackageName (mkPackageName str)) |
There was a problem hiding this comment.
[obsolete]
In my tests we only fall here when i run
cabal new-build some-package-not-in-the-project
while if I run
cabal new-build some-extra-package
we fall in the Unambiguous target case.
I don't understand if
- it should match the extra package
- it should match any other package (ex. from hackage)
If (1): this doesn't work and we probably need to re-add syntaxForm1Name and filter out extra packages in syntaxForm1Package.
If (2): Well, until now i thought we were dealing with the first case... I'll have to think abou this.
There is nothing except for syntax support, but this is a first step
towards proper support for targets refering to dependencies or to out of
project packages.
For the moment, when used, it will report:
cabal: Cannot build the package foobar, it is not in this project
(either directly or indirectly). If you want to add it to the
project then edit the cabal.project file.
Also update the test output for an integration test.
It's currently in the old Targets module, but we'll need it in the new-build code too soon, and it's not really that closely related to targets, so it makes sense to have it live in the common Types module.
That is, any package name within the install plan. This allows targeting a dependency of a package that is local to the project.
So you can now add `extra-packages: foo` to the cabal.projct file and then `cabal (new-)build foo`. The extra packages are included into the install plan and they are also resolved as build targets. Currently this only uses the "any valid package name" target syntax which means you can use `foo` but not `foo:tests` or any of the other variations.
dcoutts
force-pushed
the
target-package-names
branch
from
cd500ab to
e002ad4
Compare
|
FYI, I'm trying to improve the target parser further to make this more flexible. So I appreciate review already but hold off merging for now. |
23Skidoo
left a comment
23Skidoo
left a comment
There was a problem hiding this comment.
LGTM modulo minor bikeshedding. Any chance we can get some test cases?
| -- resolve to a dependency of a local package or to a package from | ||
| -- hackage. Either way, it requires further processing to resolve. | ||
| -- | ||
| | TargetPackageName PackageName |
There was a problem hiding this comment.
Maybe call it TargetExtraPackageName or TargetUnknownPackage to highlight the connection to extra-packages and make the distinction between it and TargetPackage more clear?
|
@23Skidoo New-install will indirectly test it |
Add CLI target support for package names. Ultimately this will be able to be used for referring to out-of-project packages, or packages that are deps of things in the project.
At the moment there is nothing except for syntax support.
For the moment, when used, it will report:
This is also intended to help @fgaz with his work. It may make sense to rebase the wip/extra-packages on these changes.
The integration-tests2 cover the new syntax to the extent that it checks ambiguity. What is missing is positive tests for the new syntax.
In addition this PR includes two simple clean-up patches.