-
Notifications
You must be signed in to change notification settings - Fork 296
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
[manifest license] fully implement SPDX, plus general parsing stuff #334
Conversation
bb16961
to
da8b942
Compare
6aeff52
to
7a83528
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.
The "request changes" is more about not enough tests for that much terrifying string manipulation code; everything else is nitpicks.
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.
Whoops, thanks VS Code.
06b31a5
to
105d0f5
Compare
src/vcpkg/sourceparagraph.cpp
Outdated
|
||
enum class Mode | ||
std::string parse_spdx_license_expression(StringView sv, std::vector<ParseSpdxLicenseError>& errors) |
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.
Why not use our existing parser framework for this (Parse::Parserbase
)?
I see two differences compared to the code you have here:
- Localization - we must eventually add Localization to ParserBase for all the other parsers in the product, so this doesn't seem like a good reason to do things completely ad-hoc here. Either this new code should use the existing framework (and get localized when we localize every other parser) or we should localize ParserBase first in a separate PR and then add this code.
- Warnings - adding warnings to the existing parsing framework seems obviously good and not long or complex.
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 didn't think about it until I was very close to done with this implementation. I can switch it over, I think, pretty easily tho.
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.
hmm, now that I'm looking at this, it seems like it'd be quite difficult to push this parser into the old ParserBase model; I would prefer to rewrite the model from scratch, and then switch stuff over as it comes up. I also don't want to add new un-localized code. Also, moving everything over to the new model would be prohibitively expensive.
cd397d2
to
c86dfce
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.
LGTM with the std::string{}
change.
This caused microsoft/vcpkg#23110 |
This includes the following features:
LicenseRef-[blah]
, for licenses which have not yet been added to SPDXnull
license, for proprietary licenses generally (this means "look at share/<port>/copyright" for license info)(a AND b) WITH c
was accepted before, and isn't nowError Examples:
Unrecognized ASCII character
=>
Unicode characters
=>
Incorrect exceptions on multiple licenses
=>
Unknown License