Skip to content
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

[vcpkg] Improve Json error messages #12981

Merged
merged 5 commits into from
Sep 7, 2020

Conversation

ras0219
Copy link
Contributor

@ras0219 ras0219 commented Aug 18, 2020

This PR improves error messages reported by the JSON parser; specifically when handling manifests.

This includes a fundamental shift away from the existing error approach taken by the CONTROL-file code because that approach is inappropriate for a JSON world. The CONTROL file format is specifically a very flat format, so explicitly printing a list of valid fields makes sense because there's only one context for fields[1].

JSON, on the other hand, is a fundamentally different beast with its hierarchical objects and arrays and therefore merits a different strategy. In this PR, I've approached the problem similar to compiler error messages which are better designed for a tree-based structure. Explaining the full possible schema in the error output would be extremely verbose, so instead I've opted to link users to the current best documentation (docs/specifications/manifests.md and docs/specifications/registries.md). When we have more polished docs for these file types, these links can be updated to point there.

To help users locate the errors within their files, I've opted to use a JSONPath-style location; potential future work would be to add source location tracking into the JSON nodes to enable proper "Go To Error" in editors such as Visual Studio Code.

As a drive by, this also fixes a regression in start-of-line tracking.

[1] Feature paragraphs are close enough to Source paragraphs that the valid field list is not great but still works.

Example 1:

{
  "name": "abseil",
  "version-string": "2020-03-03",
  "port-version": 8,
  "description": "a",
  "homepage": "https://github.com/abseil/abseil-cpp",
  "features": [
    {
      "name": "cxx17",
      "description": "Enable compiler C++17."
    }
  ],
  "dependencies": [
    [],
    "a",
    { "b": 1 }
  ]
}

Before:

PS C:\src\vcpkg\testing> ..\vcpkg-old install --dry-run --feature-flags=manifests
Error: There are invalid field types in the CONTROL or manifest file of manifest
The following fields had the wrong types:

    dependencies was expected to be an array of dependencies

Failed to parse manifest C:\src\vcpkg\testing/vcpkg.json.

After:

PS C:\src\vcpkg\testing> ..\vcpkg install --dry-run --feature-flags=manifests
Errors occurred while parsing C:\src\vcpkg\testing\vcpkg.json
    $.dependencies[0]: mismatched type: expected a dependency
    $.dependencies[2] (a dependency): unexpected field 'b'
    $.dependencies[2] (a dependency): missing required field 'name' (a package name)
See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/manifests.md for more information.

Example 2:

{
  "name": "abseil",
  "version-string": "2020-03-03",
  "port-version": 8,
  "description": "abc",
  "homepage": "https://github.com/abseil/abseil-cpp",
  "features": [
    {
      "name": "cxx17",
      "decsription": "Enable compiler C++17."
    }
  ]
}

Before:

PS C:\src\vcpkg\testing> ..\vcpkg-old install --dry-run --feature-flags=manifests
Error: There are invalid fields in the control or manifest file of manifest
The following fields were not expected:
    In a feature: decsription
This is the list of valid fields for CONTROL files (case-sensitive):

    Source
    Version
    Port-Version
    Description
    Maintainer
    Build-Depends
    Homepage
    Type
    Supports
    Default-Features

And this is the list of valid fields for manifest files:

    name
    version-string
    port-version
    maintainers
    description
    homepage
    documentation
    license
    dependencies
    dev-dependencies
    features
    default-features
    supports

You may need to update the vcpkg binary; try running .\bootstrap-vcpkg.bat to update.

Error: There are missing fields in the control file of manifest
The following fields were missing:
    In a feature: description
Failed to parse manifest C:\src\vcpkg\testing/vcpkg.json.

After:

PS C:\src\vcpkg\testing> ..\vcpkg install --dry-run --feature-flags=manifests
Errors occurred while parsing C:\src\vcpkg\testing\vcpkg.json
    $.features[0] (a feature): unexpected field 'decsription'
    $.features[0] (a feature): missing required field 'description' (a string or array of strings)
See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/manifests.md for more information.

Example 4:

{
  "name": "abseil",
  "version-string": "2020-03-03",
  "port-version": 8,
  "description": "abc",
  "homepage": "https://github.com/abseil/abseil-cpp",
  "features": [
    {
      "name": ["cxx17"],
      "description": "Enable compiler C++17."
    }
  ]
}

Before:

PS C:\src\vcpkg\testing> ..\vcpkg-old install --dry-run --feature-flags=manifests
Error: There are invalid field types in the CONTROL or manifest file of manifest
The following fields had the wrong types:

    name was expected to be an identifier

Failed to parse manifest C:\src\vcpkg\testing/vcpkg.json.

After:

PS C:\src\vcpkg\testing> ..\vcpkg install --dry-run --feature-flags=manifests
Errors occurred while parsing C:\src\vcpkg\testing\vcpkg.json
    $.features[0].name: mismatched type: expected an identifier
See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/manifests.md for more information.

Example 5:

{
  "name": "abseil",
  "version-string": "2020-03-03",
  "port-version": 8,
  "description": "abc",
  "homepage": "https://github.com/abseil/abseil-cpp",
  "features": [
    {
      "name": "cxx17",
      "description": "Enable compiler C++17.",
    }
  ]
}

Before:

PS C:\src\vcpkg\testing> ..\vcpkg-old install --dry-run --feature-flags=manifests
Failed to parse manifest at C:\src\vcpkg\testing\vcpkg.json: Error: C:/src/vcpkg/testing/vcpkg.json:11:7: Trailing comma in an object
   on expression: {
    "name": "abseil",
    "version-string": "2020-03-03",
    "port-version": 8,
    "description": "abc",
    "homepage": "https://github.com/abseil/abseil-cpp",
    "features": [
      {
        "name": "cxx17",
        "description": "Enable compiler C++17.",
      }

                                                    ^

After:

PS C:\src\vcpkg\testing> ..\vcpkg install --dry-run --feature-flags=manifests
Failed to parse manifest at C:\src\vcpkg\testing\vcpkg.json:
Error: C:/src/vcpkg/testing/vcpkg.json:10:48: Trailing comma in an object
   on expression:         "description": "Enable compiler C++17.",
                                                                 ^

Example 6 (vcpkg-configuration.json):

{
    "a":1
}

Before:

PS C:\src\vcpkg\testing> ..\vcpkg-old install --dry-run --feature-flags=manifests
Error: Invalid fields in configuration:
    a configuration object had invalid fields: a

After:

PS C:\src\vcpkg\testing> ..\vcpkg install --dry-run --feature-flags=manifests    
Errors occurred while parsing C:\src\vcpkg\testing\vcpkg-configuration.json
    $ (a configuration object): unexpected field 'a'
See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/registries.md for more information.

Example 7:

{
    "name": "abseil",
    "version-string": "2020-03-03",
    "description": "",
    "dependencies": [
        { "name": "foo", "paltform": "linux", "platform": "linux" }
    ]
}

Before (Note: the valid field 'platform' is not listed anywhere in the list of valid fields):

PS C:\src\vcpkg\testing> ..\vcpkg-old install --dry-run --feature-flags=manifests
Error: There are invalid fields in the control or manifest file of manifest
The following fields were not expected:
    In a dependency: paltform
This is the list of valid fields for CONTROL files (case-sensitive):

    Source
    Version
    Port-Version
    Description
    Maintainer
    Build-Depends
    Homepage
    Type
    Supports
    Default-Features

And this is the list of valid fields for manifest files: 

    name
    version-string
    port-version
    maintainers
    description
    homepage
    documentation
    license
    dependencies
    dev-dependencies
    features
    default-features
    supports

You may need to update the vcpkg binary; try running .\bootstrap-vcpkg.bat to update.

Failed to parse manifest C:\src\vcpkg\testing/vcpkg.json.

After:

PS C:\src\vcpkg\testing> ..\vcpkg install --dry-run --feature-flags=manifests    
Errors occurred while parsing C:\src\vcpkg\testing\vcpkg.json
    $.dependencies[0] (a dependency): unexpected field 'paltform'
See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/manifests.md for more information.

@strega-nil strega-nil self-requested a review August 18, 2020 18:36
@strega-nil strega-nil self-assigned this Aug 18, 2020
Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only thing I see that's easy to change. However, I have a lot of changes in #13038 that I don't particularly wish to merge with this, since they both make a lot of the same changes. I'd rather merge that one first because this is going to be easier to merge later.

toolsrc/include/vcpkg/base/json.h Show resolved Hide resolved
@ras0219 ras0219 marked this pull request as draft August 28, 2020 18:53
@ras0219
Copy link
Contributor Author

ras0219 commented Aug 28, 2020

Converting to draft until #13038 has been merged according to @strega-nil's preference

@ras0219 ras0219 marked this pull request as ready for review September 4, 2020 08:51
Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really great changeset :D

toolsrc/src/vcpkg/install.cpp Show resolved Hide resolved
@ras0219
Copy link
Contributor Author

ras0219 commented Sep 5, 2020

I reverted the line break change because it does not comply with our clang-format rules; if we want to change the rules I think it would be best to do it in another PR.

@ras0219-msft ras0219-msft merged commit 0d0a846 into microsoft:master Sep 7, 2020
@ras0219-msft
Copy link
Contributor

Thanks for the reviews!

strega-nil pushed a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
* [vcpkg] Fix error reporting on json parse failure

* [vcpkg] Track manifest path for use in diagnostics

* [vcpkg] Use by-value for consumer API. Improve trailing comma diagnostic.

* [vcpkg] Track errors directly inside Json::Reader

* [vcpkg] Fixup use of .u8string()

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants