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

Add SPDX 2.2.1 support (SBOM) #296

Merged
merged 8 commits into from
Apr 1, 2022

Conversation

ras0219-msft
Copy link
Contributor

@ras0219-msft ras0219-msft commented Dec 7, 2021

See https://spdx.github.io/spdx-spec/ for more details.

Depends on #323

@ras0219-msft ras0219-msft force-pushed the dev/roschuma/sbom branch 2 times, most recently from 140c546 to ea355cc Compare December 7, 2021 23:08
@ras0219-msft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil-ms
Copy link
Contributor

This is an incredible amount of code for just updating to SPDX 2.2.1; what's actually going on here?

include/vcpkg/base/json.h Outdated Show resolved Hide resolved
include/vcpkg/uuid.h Outdated Show resolved Hide resolved
include/vcpkg/base/strings.h Outdated Show resolved Hide resolved
include/vcpkg/base/expected.h Show resolved Hide resolved
src/vcpkg/build.cpp Show resolved Hide resolved
}

static bool is_cmake_whitespace(int ch) { return ch == ' ' || ch == '\n' || ch == '\t' || ch == '\r'; }
static bool is_cmake_identifier(int ch)
Copy link
Contributor

@strega-nil strega-nil Feb 2, 2022

Choose a reason for hiding this comment

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

what is this for? this function does not include all of CMake's "identifiers" (by which I assume you mean "identifiers that can be included in ${...} syntax"). It is missing /_+-, and escape sequences.

edit: oh, identifier is what CMake calls their command names. I'd like a comment here if possible.

Also, this should take a char32_t or raw char.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intent with these "heuristics" was entirely as a point-in-time/proof-of-concept stop gap until our standard helpers can be instrumented to emit resource documents. At that point, these heuristics should be retired since they're hopelessly outclassed (can't handle variable substitution, can't detect data-dependent paths).

Therefore I wanted to minimize sharing and refactoring of other parts of the codebase, since I hope to delete these entirely.

src/vcpkg/spdx.cpp Outdated Show resolved Hide resolved
src/vcpkg/spdx.cpp Outdated Show resolved Hide resolved
src/vcpkg/spdx.cpp Outdated Show resolved Hide resolved
obj.insert("downloadLocation", "NONE");
obj.insert("licenseConcluded", conclude_license(cpgh.license.value_or("")));
obj.insert("licenseDeclared", noassert);
obj.insert("copyrightText", noassert);
Copy link
Contributor

Choose a reason for hiding this comment

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

should this include <installed>/share/<port>/copyright?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I wanted to get basic stuff in place and see what additional data is required after. For example, maybe we can use the SPDX license "go look in that file" instead of having to copy the entire text.

@strega-nil-ms strega-nil-ms changed the title Add SPDX 2.2.1 support (SBOM) [Work in progress] Add SPDX 2.2.1 support (SBOM) Feb 2, 2022
@strega-nil-ms strega-nil-ms marked this pull request as ready for review February 2, 2022 01:02
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

Approve with nits fixed

include/vcpkg/base/expected.h Show resolved Hide resolved
src/vcpkg-test/util.cpp Show resolved Hide resolved
src/vcpkg/spdx.cpp Outdated Show resolved Hide resolved
src/vcpkg/spdx.cpp Outdated Show resolved Hide resolved
src/vcpkg/spdx.cpp Outdated Show resolved Hide resolved
src/vcpkg/spdx.cpp Outdated Show resolved Hide resolved
@ras0219-msft ras0219-msft merged commit fff77ff into microsoft:main Apr 1, 2022
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