[spdx] Fix several bugs #1377
[spdx] Fix several bugs #1377
Conversation
There was a problem hiding this comment.
Overall, I am really happy with the improvements in this PR! However, I think there's a few aspects of the implementation that need to be addressed.
This PR adds additional SPDX features but no SPDX tests. I'd like to see tests covering vcpkg::run_resource_heuristics for each of the new helpers. Adding tests to cover the existing vcpkg_from_github would be appreciated but not necessary :)
Additionally, can you please include a sample of SPDX documents before and after your change for a handful of ports, demonstrating that all changes were intentional and positive?
Indexing of SPDXRef-resource started at 1
This is not a bug, unless you can reference an SPDX specification that indicates it should start at 0?
src/vcpkg-test/cmake.cpp
Outdated
| { | ||
| auto res = extract_cmake_invocation_argument("lorem \"ipsum\"", "lorem"); | ||
| REQUIRE(res == "ipsum"); | ||
| } |
There was a problem hiding this comment.
Can we add additional testing that covers:
- More quote situations (
lorem \"ipsum\\\"\",lorem \"ipsum spaces\") - Multiple invocations (
alligator bee cactus dog elephantsearching forcactusofdog) - Case (in)sensitivity
include/vcpkg/cmake.h
Outdated
|
|
||
| namespace vcpkg | ||
| { | ||
| StringView find_cmake_invocation(StringView content, StringView command); |
There was a problem hiding this comment.
Given that these functions are very much heuristics and only usable with caveats, I don't think they should be introduced as an independent "cmake" module. Instead, I think they should remain in spdx.cpp and exposed as needed for testing purposes.
Ideally, we will get rid of these heuristics entirely and instead introduce some sort of "side channel" that allows the portfile to record accesses -- such as by writing out a json document inside vcpkg_from_github() with the actual information.
If we do introduce another area that needs CMake parsing heuristics before we're able to delete them from the SPDX path, we can move them out into separate files at that point.
src/vcpkg-test/cmake.cpp
Outdated
| REQUIRE(res.empty()); | ||
| } | ||
| { | ||
| auto res = find_cmake_invocation("lorem_ipsum( )", "lorem_ipsum"); |
There was a problem hiding this comment.
This should test additional edge cases:
lorem_ipsum_lorem_ipsum (lorem_ipsum(EOF)
# Conflicts: # src/vcpkg/spdx.cpp
…by said test cases.
| if (it == contents.end()) return {}; | ||
| it += command.size(); | ||
| if (it == contents.end()) return {}; | ||
| if (ParserBase::is_word_char(*it)) return {}; |
There was a problem hiding this comment.
@ras0219-msft I really don't understand the significance of these 'word char' checks here. If a file has vcpkg_from_github .... vcpkg_from_git, then we should skip over the first one and go to the real one, which I added a test for and fixed. Please double check that I did not misunderstand these' purpose.
Fixed bugs:
vcpkg_from_gitdidn't work because it used the results of parsingvcpkg_from_githubvcpkg_from_bitbucketandvcpkg_from_gitlabweren't parsedSPDXRef-resourcestarted at 1vcpkg_from_*/vcpkg_download_distfilewas parsedI.e., if there were 2 occurences of
vcpkg_from_github, only the first one was included in the SBOM file.Changes in behavior:
find_cmake_invocationno longer uses case insensitive comparison