Track cabal.project provenance for error reporting #3844

brendanhay · 2016-09-15T08:37:17Z

This changes the error when a cabal.project is missing from:

% cabal new-build
The package location glob 'foo./*.cabal' does not match any files or directories.

To (Implicit):

% cabal new-build
When using an implicit cabal.project configuration, the following errors were encountered:
The package location glob './*.cabal' does not match any files or directories.

Or (Explicit):

% cabal new-build
When using the configuration located at /home/bren/proj/cabal/cabal.project, the following errors were encountered:
The package location glob './*.cabal' does not match any files or directories.

This is done by tracking the provenance of the cabal.project file in readProjectLocalConfig, otherwise it defaults to Implicit.

The ProjectConfig is now also passed to BadPackageLocations and can be utilised in renderBadPackageLocation, although only the provenance is used currently to formulate a title specifying what configuration was used. This will hopefully allow individual calls to renderBadPackageLocation to be much more informative.

Note: I'm not certain the Semigroup and Monoid instances make sense for ProjectConfigProvenance.

Fixes #3636

mention-bot · 2016-09-15T08:37:18Z

@brendanhay, thanks for your PR! By analyzing the annotation information on this pull request, we identified @dcoutts, @hvr and @ezyang to be potential reviewers

brendanhay · 2016-09-15T09:18:13Z

I notice a couple of the ProjectConfig properties are failing, I'll look into it shortly.

dcoutts · 2016-09-15T11:15:45Z

Great!

I think we can probably improve the wording further, especially in the implicit case. In the implicit case we can match on the common kinds of errors and give a specific message.

ezyang · 2016-09-15T13:35:22Z

Thank you @brendanhay, provenance tracking is a great idea and will help our error messages.

You comment that you are not sure the Semigroup/Monoid instances make sense. I think this is indicative of deeper problems with the representation you have chosen. The big problem is that two ProjectConfigs can be merged together and now your provenance information is a bit scrambled. Provenance should not be a single ProjectConfigProvenance; it should be a set, tracking all of the ProjectConfigs that were inputs to the merge. Now you don't need a Semigroup/Monoid instance for ProjectConfigProvenance.

This also points out that your provenance tracking might be too coarse. When we renderBadPackageLocations, we don't care about the provenance of the config file as a whole; we just want to know which config file contributed the bad one. So really BadPackageLocation should be annotated with a location.

If we follow this line of reasoning to its logical conclusion, it would seem that we should refactor many of the types in cabal-install to be located, in the same way that GHC associates every Haskell-level expression with a line and column number. That would be a big refactor, but a worthwhile one, in my opinion. But let us not let perfect be the enemy of good. If there's a bad package location, giving the user a list of config files to look at to find the offending one is better than nothing at all.

To summarize: EITHER change ProjectConfigProvenance into Set ProjectConfigProvenance and drop the bogus Monoid/Semigroup instances, OR refactor ProjectConfig and the data types it refers to so that everything is located. (But not the Cabal library types, because that would be BC-breaking.)

brendanhay · 2016-09-15T13:54:54Z

@ezyang Yes, that makes much more sense.

I interpret your comment as meaning your long-term preference is to have the location information, In which case one suggestion I have is to implement the Set ProjectConfigProvenance as a resolution to this PR, and then to work on adding location information as a separate PR which will then possibly supersede some of these changes.

ezyang · 2016-09-15T14:00:07Z

Yes. I wanted to mention the long term refactor because, as you say, it does supersede the easier version. But in this case the easy fix is pretty easy, so it shouldn't be too bad to throw out later.

P.S., the test failure is because provenance isn't preserved if you print out a config, and then read it back in. I'm not sure if this means you should print out the provenance, or just ignore provenance when testing if it was roundtripped. Given the long term plan, probably the latter?

brendanhay · 2016-09-15T14:45:07Z

Both SavedConfig and LegacyProjectConfig would be represented by mempty :: Set ProjectConfigProvenance, but what about defaultImplicitProjectConfig? Is it explicitly Set.singleton Implicit (drumroll) or can that also be considered to be equivalent to an empty Set?

In which case, would it make more sense to use a type such as Set FilePath to track provenance with the empty set denoting both legacy configuration, mempty / default configuration, or anything excluding a configuration that was explicitly read from a FilePath location?

An alternative would be to track with even more granularity things such as default configuration created via mempty vs defaultImplicitProjectConfig and so on. Although it seems that way has escher-esque potential and should really be left to more general/pervasive type location annotations.

ezyang · 2016-09-15T14:59:04Z

In situations like this, I find it is useful to remember that mempty has a very specific meaning: it must be that mappend mempty e is e. So mempty MUST be the empty set.

On the other hand, it's fine for the implicit config to come with a provenance. It actually would be quite useful, because the implicit config is NOT used when there's an explicit config. So if there was an error related to implicit config, we could give an appropriate message in that case.

ezyang

See discussion.

ezyang · 2016-09-16T15:51:00Z

cabal-install/Distribution/Client/ProjectConfig.hs

      }

+    setExplicitProjectConfig :: ProjectConfig -> ProjectConfig
+    setExplicitProjectConfig config =


Are you sure you don't want to make a helper function for this operation? The helper could be more robust: instead of obliterating the old provenance it would just add to it (unless obliterating it is what you want here? Hard to tell.)

When you say 'helper function' I'm going to assume you mean moving it to the top-level, which since it's not being used elsewhere it's nice (IMO) to have that apparent by its location in a where clause.

The old provenance should indeed be preserved via Set.insert.

ezyang · 2016-09-16T15:51:42Z

cabal-install/Distribution/Client/ProjectConfig.hs

+-- | Render bad package location error information for the implicit
+-- @cabal.project@ configuration.
+--
+-- TODO: This is currently a placeholder with the intention to supply


OK, well, let's not forget about the todo :)

ezyang · 2016-09-16T15:52:08Z

cabal-install/Distribution/Client/ProjectConfig/Types.hs

+-- used or if the configuration was read from an explicit file path.
+data ProjectConfigProvenance
+
+     -- | The project configuration was implied.


Implied by what? If I read this, I would have no idea where to look to find out more. Crossref please.

ezyang · 2016-09-16T15:53:47Z

@brendanhay For future reference, participants don't get emails when you push new commits to the branch, you'll have to make a comment to notice it.

brendanhay · 2016-09-16T17:29:08Z

Yeah, was aware my sneaky commits wouldn't be noticed. Still want to add some actual nice errors where the TODO is first. I'll incorporate the other changes you suggested too.

brendanhay · 2016-09-17T07:57:54Z

cabal-install/Distribution/Client/ProjectConfig.hs

+     ++ "Please create a package description file <pkgname>.cabal "
+     ++ "or a cabal.project file referencing the packages you  "
+     ++ "want to build."
+    _ -> renderBadPackageLocation bpl


No cabal.project or cabal file matching the glob './*.cabal' was found. Please create a package description file <pkgname>.cabal or a cabal.project file referencing the packages you want to build.

Regarding the other cases, I'm not convinced I actually understand well enough under what situations these errors are raised to correctly word the implicit error message.

ezyang

Still looking good.

ezyang · 2016-09-17T12:23:25Z

cabal-install/Distribution/Client/ProjectConfig.hs

+      -- per package error.
+    | otherwise =
+           "When using configuration(s) from "
+        ++ intercalate ", " (mapMaybe getExplicit provenance)


Maybe this should also mention if the implicit is used? (I think this case is impossible, so outputting something in that case could help debugging in the future. Or maybe even an error.)

ezyang · 2016-09-17T12:25:59Z

cabal-install/Distribution/Client/ProjectConfig.hs

+renderImplicitBadPackageLocation :: BadPackageLocation -> String
+renderImplicitBadPackageLocation bpl = case bpl of
+    BadLocGlobEmptyMatch pkglocstr ->
+        "No cabal.project or cabal file matching the glob '"


Perhaps say "default glob" rather than "glob"? Because it doesn't actually exist anywhere.

- Excludes provenance from all roundtrip tests - Inserting explicit provenance when read from file - Special cases for bad package errors arising from implicit project configuration

ezyang mentioned this pull request Sep 15, 2016

You're awesome! We're giving you commit bits. #3567

Closed

ezyang added the blocked: action-required label Sep 15, 2016

ezyang added this to the 2.0 milestone Sep 15, 2016

ezyang requested changes Sep 16, 2016

View reviewed changes

ezyang approved these changes Sep 16, 2016

View reviewed changes

brendanhay commented Sep 17, 2016

View reviewed changes

ezyang reviewed Sep 17, 2016

View reviewed changes

ezyang removed the blocked: action-required label Sep 17, 2016

Track project configuration provenance

c2ebd71

- Excludes provenance from all roundtrip tests - Inserting explicit provenance when read from file - Special cases for bad package errors arising from implicit project configuration

brendanhay merged commit d1d9737 into haskell:master Sep 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track cabal.project provenance for error reporting #3844

Track cabal.project provenance for error reporting #3844

brendanhay commented Sep 15, 2016

mention-bot commented Sep 15, 2016

brendanhay commented Sep 15, 2016

dcoutts commented Sep 15, 2016

ezyang commented Sep 15, 2016 •

edited

Loading

brendanhay commented Sep 15, 2016

ezyang commented Sep 15, 2016 •

edited

Loading

brendanhay commented Sep 15, 2016

ezyang commented Sep 15, 2016

ezyang left a comment

ezyang Sep 16, 2016

brendanhay Sep 17, 2016

ezyang Sep 16, 2016

ezyang Sep 16, 2016

ezyang commented Sep 16, 2016

brendanhay commented Sep 16, 2016

brendanhay Sep 17, 2016

brendanhay Sep 17, 2016

ezyang left a comment

ezyang Sep 17, 2016

ezyang Sep 17, 2016

Track cabal.project provenance for error reporting #3844

Track cabal.project provenance for error reporting #3844

Conversation

brendanhay commented Sep 15, 2016

mention-bot commented Sep 15, 2016

brendanhay commented Sep 15, 2016

dcoutts commented Sep 15, 2016

ezyang commented Sep 15, 2016 • edited Loading

brendanhay commented Sep 15, 2016

ezyang commented Sep 15, 2016 • edited Loading

brendanhay commented Sep 15, 2016

ezyang commented Sep 15, 2016

ezyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezyang commented Sep 16, 2016

brendanhay commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezyang commented Sep 15, 2016 •

edited

Loading

ezyang commented Sep 15, 2016 •

edited

Loading