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

[ScanDependencies] Fix JSON generation under certain circunstances. #67246

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

drodriguez
Copy link
Collaborator

The code of ScanDependencies.cpp was creating invalid JSON since #66031 because in the case of having extraPcmArgs and swiftOverlayDependencies, but not bridgingHeader, a comma will not be added at the end of extraPcmArgs, creating an invalid JSON file. Additionally that same PR added a trailing comma at the end of the swiftOverlayDependencies, which valid JSON does not allow, but that bug was removed in #66366.

Both problems are, however, present in the 5.9 branch, because #66936 included #66031, but not #66366.

Besides fixing the problem in ScanDependencies.cpp I modified every test that uses --scan-dependencies to pass the produced JSON through Python's json.tool in order to validate proper JSON is produced. In most cases I was able to pipe the output of the tool into FileCheck, but in some cases the validation is done by itself because the checks depend on the exact format generated by --scan-dependencies. In a couple of tests I added a call to FileCheck that seemed to be missing.

Without these changes, two tests seems to be generating invalid JSON in my machine:

  • ScanDependencies/local_cache_consistency.swift (which outputs Expecting ',' delimiter: line 525 column 11 (char 22799))
  • ScanDependencies/placholder_overlay_deps.swift

@drodriguez
Copy link
Collaborator Author

@swift-ci please test

@artemcm
Copy link
Contributor

artemcm commented Jul 12, 2023

@drodriguez did you accidentally omit the actual code change from the PR?

@drodriguez
Copy link
Collaborator Author

I did indeed. It was late at night and I was testing the test changes by themselves. I will look at those Windows errors and resubmit in a moment.

The code of `ScanDependencies.cpp` was creating invalid JSON since apple#66031
because in the case of having `extraPcmArgs` and `swiftOverlayDependencies`,
but not `bridgingHeader`, a comma will not be added at the end of
`extraPcmArgs`, creating an invalid JSON file. Additionally that same PR
added a trailing comma at the end of the `swiftOverlayDependencies`, which
valid JSON does not allow, but that bug was removed in apple#66366.

Both problems are, however, present in the 5.9 branch, because apple#66936
included apple#66031, but not apple#66366.

Besides fixing the problem in `ScanDependencies.cpp` I modified every test
that uses `--scan-dependencies` to pass the produced JSON through
Python's `json.tool` in order to validate proper JSON is produced. In
most cases I was able to pipe the output of the tool into `FileCheck`,
but in some cases the validation is done by itself because the checks
depend on the exact format generated by `--scan-dependencies`. In
a couple of tests I added a call to `FileCheck` that seemed to be
missing.

Without these changes, two tests seems to be generating invalid JSON in
my machine:

- `ScanDependencies/local_cache_consistency.swift` (which outputs `Expecting ',' delimiter: line 525 column 11 (char 22799)`)
- `ScanDependencies/placholder_overlay_deps.swift`
@drodriguez
Copy link
Collaborator Author

@swift-ci please test

Copy link
Contributor

@artemcm artemcm left a comment

Choose a reason for hiding this comment

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

Thank you!

@artemcm
Copy link
Contributor

artemcm commented Jul 12, 2023

@drodriguez would you kindly also open a cherry-pick PR against release/5.9?

@drodriguez
Copy link
Collaborator Author

@swift-ci please test windows platform

@drodriguez drodriguez merged commit 70376a1 into apple:main Jul 12, 2023
5 checks passed
@drodriguez drodriguez deleted the validate-json branch July 12, 2023 21:19
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.

None yet

2 participants