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

Attempt conversion of SPDX license expressions to Homebrew DSL #1345

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

cxreiff
Copy link
Contributor

@cxreiff cxreiff commented Aug 19, 2024

Resolves #820.

Problem

Currently, when publishing to a Homebrew tap, whatever string is in the crate's Cargo.toml license field (usually a SPDX license expression) gets passed into license in the Homebrew formula. However, rather than using SPDX syntax, Homebrew has its own ruby syntax for specifying licenses. So, when passed an SPDX expression, Homebrew:

  • Cannot provide as detailed information about the package's licensing.
  • Throws errors when linting the formula with brew audit/brew test-bot.

Solution

  • Use the spdx crate to parse the SPDX license expression.
  • Iterate through the resulting token stream to construct a string matching Homebrew's license domain-specific language.
  • Pass the result into Homebrew's license command, falling back to the original license string if there are issues parsing.

Scope

  • This PR just handles OR/AND operators, as the spdx crate has not yet implemented v3 of the spec. This means that the WITH operator is not yet covered, and neither are lower-case versions of the operators.
    • spdx issue for implementing spec v3 is here.
  • This PR formats everything on one line- in combination with PR 1340 this shouldn't be an issue, as it will be auto-formatted.

Testing

I added tests for the conversion utility function, tested the output in a Homebrew formula, and I inspected and updated the test snapshots. This PR could certainly use some additional testing from someone with deeper familiarity with the project.

Copy link
Contributor

@mistydemeo mistydemeo 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 so much! This is quite thorough - I appreciate it. Just a couple of comments.

cargo-dist/src/backend/installer/homebrew.rs Show resolved Hide resolved
cargo-dist/src/backend/installer/homebrew.rs Outdated Show resolved Hide resolved
cargo-dist/src/backend/installer/homebrew.rs Show resolved Hide resolved
Copy link
Contributor

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

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

Looking good! Just noticed that two conditions here are parsed differently from how Homebrew does it - wanted to check in and see how you're feeling about that at this point.

cargo-dist/src/backend/installer/homebrew.rs Show resolved Hide resolved
cargo-dist/src/backend/installer/homebrew.rs Show resolved Hide resolved
added `spdx` dependency for parsing license expression.
added to_homebrew_license_format utility function with tests.
modified add_homebrew_installer to attempt conversion
updated snapshots
@cxreiff cxreiff force-pushed the homebrew-multiple-license branch from 507f9e7 to b9ceed3 Compare August 20, 2024 21:12
Copy link
Contributor

@mistydemeo mistydemeo 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!

@mistydemeo mistydemeo merged commit a663e04 into axodotdev:main Aug 20, 2024
15 checks passed
mistydemeo added a commit that referenced this pull request Aug 21, 2024
These two tests produce different results in Homebrew's SPDX parser,
but that appears to be a bug in Homebrew's parser rather than this one.
Added notes to both tests so that we expect the results to be different
from what Homebrew returns.

refs #1345 (comment).
@cxreiff cxreiff deleted the homebrew-multiple-license branch August 21, 2024 00:05
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.

Homebrew: improve license field handling
2 participants