-
Notifications
You must be signed in to change notification settings - Fork 718
REPL command in project requires a target #10684
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
base: master
Are you sure you want to change the base?
REPL command in project requires a target #10684
Conversation
09fcf06
to
6dd178f
Compare
2782ef9
to
5a50424
Compare
35477a0
to
1b8edc1
Compare
1b8edc1
to
2a587e3
Compare
2a587e3
to
59869f3
Compare
59869f3
to
d34ecf2
Compare
d34ecf2
to
80875e2
Compare
This pull request will be easier to review once the |
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.
Approving despite suggestions because they're not functional changes.
2e359e9
to
88c7a58
Compare
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.
Thanks for this patch and the test! I have a couple questions...
@mpickering could you take a brief look at this (it's a small patch)? Especially my comment under the -- NOTE:
.
text "There are no packages in" | ||
<+> (project <> char '.') | ||
<+> text "Please add a package to the project and pick a component to use as the target of the REPL command." |
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.
the no-packages case is certainly already handled in other places today, right? (people keep forgetting packages:...
and get an error message.) Is it possible to not duplicate this logic?
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.
After removing the target string manipulation, the current behaviour (#9983) is bad and doesn't cover this case:
$ ~/.ghcup/bin/cabal --numeric-version
3.14.2.0
$ ~/.ghcup/bin/cabal repl --project-dir=cabal-testsuite/PackageTests/ReplProjectTarget --project-file=empty.project
Configuration is affected by the following files:
- empty.project
Warning: There are no packages or optional-packages in the project
Resolving dependencies...
Error: [Cabal-7076]
Internal error when trying to open a repl for the package fake-package-0. The package is not in the set of available targets for the project plan, which would suggest an inconsistency between readTargetSelectors and resolveTargets.
Also I'd be curious to know how to get better output from a debug build of cabal-install with assertions enabled because that version gives me a poorer error report with no line number:
$ cabal repl --project-dir=cabal-testsuite/PackageTests/ReplProjectTarget --project-file=empty.project
Warning: this is a debug build of cabal-install with assertions enabled.
Configuration is affected by the following files:
- empty.project
Warning: There are no packages or optional-packages in the project
Resolving dependencies...
Assertion failed
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.
Warning: There are no packages or optional-packages in the project
is a different issue, I think, (#8679 probably) so it may be wise to try to avoid solving it 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.
assertion failure is bad but also looks like a future work. We're minutes from cutting the 3.16 branch and all backporting will have to go through extra careful consideration (mostly deciding the tradeoff: scarse resource for the release vs profit from performing the backport)
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.
The assertion bothers me. I could cut back on the scope of this pull request or dig in more about the assertion.
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.
the no-packages case is certainly already handled in other places today, right? (people keep forgetting
packages:...
and get an error message.) Is it possible to not duplicate this logic?
@ulysses4ever, it is handled elsewhere but as a warning:
cabal/cabal-install/src/Distribution/Client/ProjectOrchestration.hs
Lines 312 to 314 in 27593dc
-- https://github.com/haskell/cabal/issues/6013 | |
when (null (projectPackages projectConfig) && null (projectPackagesOptional projectConfig)) $ | |
warn verbosity "There are no packages or optional-packages in the project" |
So in the meantime (as I'm not suppressing the warning) we get both a warning and an error:
$ cabal repl --project-file=empty.project
Warning: this is a debug build of cabal-install with assertions enabled.
Warning: There are no packages or optional-packages in the project
Error: [Cabal-7076]
There are no packages in 'empty.project'. Please add a package to the project and pick a
single [package:][ctype:]component as target for the REPL command.
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.
Given the warning (unless there's already some way to suppress it, shouldn't the error message change to be a follow-on to 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.
I'm OK with the warning and then the error. Scolding, I warned you and now look what has happened, an error!
I feel the better way to fix this would be to suppress the warning.
Let me know if you need some help with this @philderbeast |
a49a505
to
c9d2b55
Compare
Thanks @mpickering. I'd like to be able to get a line number for the assertion I'm triggering here on this branch. Is that possible? Assertion failed with backtrace for
|
@philderbeast I run the test which is failing
which points to this code:
Is that what you are looking for? |
c9d2b55
to
354f434
Compare
Interesting. Shouldn't all CI runs have a backtrace that points to the same location for the failing assertion? Validate ubuntu-22.04 ghc-9.8.4 has the backtrace location you shared @mpickering but Validate ubuntu-22.04 ghc-9.12.2 doesn't. Here's a quick diff of a snippet of the output from those above CI runs (ghc-9.8.4 first in red). - Assertion failed
- CallStack (from HasCallStack):
- assert, called at src/Distribution/Client/ProjectOrchestration.hs:1061:3 in cabal-install-3.17.0.0-inplace:Distribution.Client.ProjectOrchestration
+ | Assertion failed
+ |
+
+ HasCallStack backtrace:
+ throwIO, called at src/Test/Cabal/Server.hs:213:21 in cabal-testsuite-3-inplace:Test.Cabal.Server |
354f434
to
9daf2b5
Compare
Interesting.. I will try when I have a moment to see why this is. I wonder if it's to do with the callstack propagation changes in 9.12.2. |
@philderbeast I reproduced and fixed the issue with assertions not showing a source location
I can make a ticket and PR next week. |
Awesome thanks @mpickering. |
cd67791
to
5271f81
Compare
@mpickering @philderbeast this patch may fix a regression that people are actively noticing in 3.16.0.0. What's needed to make progress on it, do you have an idea? |
396add4
to
0f39587
Compare
@ulysses4ever I've pushed an update where I guard against triggering the assertion. |
- Show packages when no --project-file is given - Handle the case with no packages in the project - Add a changelog - Add tests - Satisfy hlint - Satisfy fourmolu - Don't need a target when there is one package - Adjust target strings for sole package - Add alt.project tests for ReplOptions - Silence the 1st withCtx call's verbosity - Don't repeat configuration is affected by - Satisfy whitespace - Fixups after rebase - Remove punct variable
- Comma with but joining indep' clauses - Use single package Co-Authored-By: brandon s allbery kf8nh <[email protected]>
01140ec
to
8d8d6d6
Compare
Cool, thanks! Let's see what CI says. Any chance you could squash some of those commits? |
8d8d6d6
to
6f3f178
Compare
I missed updating a test. The update in the test output is: $ git diff
...
# checking repl command with an 'empty.project' with no packages
# cabal repl
Warning: There are no packages or optional-packages in the project
-Resolving dependencies...
+Error: [Cabal-7076]
+There are no packages in 'empty.project'. Please add a package to the project and pick a
+ single [package:][ctype:]component as target for the REPL command. |
6f3f178
to
f23c5be
Compare
- Improve test descriptions - Mention [package:][ctype:]component - Don't repeat [package:][ctype:]component - Lift validatedTargets, rename r as replFlags - Don't use -XRecordWildCards for configFlags - Add a one.project one pkg test - Remove target string manipulation - Make reportProjectNoTarget a function - Redo ReplProjectTarget tests - Redo ReplProjectNoneTarget tests - Satisfy fix-whitespace - Error whether or not project has packages - Guard against triggering an assertion if targets are null
f23c5be
to
b16a443
Compare
Fixes #10527 and #9983. Depend-on: #10688.
Doesn't allow an empty list of targets with a project context.
With a project, the REPL command requires a target. If one is not given then a message is shown explaining this and naming the project if the
--project-file
option was given (but not when the default 'cabal.project' project name is used implicitly).We're not yet able to list project targets so in the meantime, the messages lists the packages of the project.cabal.project
is used:--project-file
option is used, the file name is included:fake-package-0
. This was confusing:cabal-install:exe:cabal
versions mentioned usingall
as the target but this won't work for the REPL command:significance: significant
in the changelog file.