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

lib.meta.licensesSpdx: mapping from SPDX ID to compatible licenses #333953

Merged
merged 2 commits into from
Aug 25, 2024

Conversation

mibmo
Copy link
Contributor

@mibmo mibmo commented Aug 11, 2024

Description of changes

Adds a mapping from SPDX IDs to the licenses defined in lib.licenses, available in lib.licenses-spdx.

I've needed this several times, so figured I'd add it to the lib. Do let me know if this is appropriate! :)

This is my first non-package contribution to Nix, so of course any and all feedback is welcome! :) (although hopefully there isn't much to say about such a simple change)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@infinisil
Copy link
Member

Would the recently-merged getLicenseFromSpdxIdOr by @ShamrockLee already work for you too?

@mibmo
Copy link
Contributor Author

mibmo commented Aug 13, 2024

Would the recently-merged getLicenseFromSpdxIdOr by @ShamrockLee already work for you too?

Not sure how I missed that, but it's super useful! It'll work for the current PR I'm working on that needed this, so I'm perfectly fine with closing, although I do think it's useful to expose nonetheless (though I do realise the quality of this PR, so it'd have to be a different one regardless) :) But perhaps the spdxLicenses variable from the mentioned PR should just be exposed instead? It'd also make the getLicenseFromSpdxId{,Or} implementations trivial. I'd be happy to make such a PR. :)

@mibmo mibmo marked this pull request as draft August 13, 2024 15:41
@ShamrockLee
Copy link
Contributor

If we could go back in time, I might prefer lib.licenses-spdx over the function.

@infinisil Is there an idiomatic approach in Nixpkgs when referencing an attribute set mapping?

@mibmo
Copy link
Contributor Author

mibmo commented Aug 14, 2024

If we could go back in time, I might prefer lib.licenses-spdx over the function.

@infinisil Is there an idiomatic approach in Nixpkgs when referencing an attribute set mapping?

Honestly I think they both have their uses. What do you think about changing this to something like lib.meta.spdxLicenses and basing the getLicenseFromSpdx{,Or} functions on it?


I doubt it really matters for a function like this (simple task; so few licenses; and afaik the thunk should be cached anyway).... but does the implementation matter much? I'm not sure which of the two implementations are more performant here — and regardless it seems like premature optimization, but nix isn't exactly known to be performant, so better safe than sorry?

# getLicenseFromSpdxIdOr
lib.mapAttrs
  (id: ls: assert lib.length ls == 1; builtins.head ls)
  # why the groupBy? when would there be duplicate licenses? (if this is a general concern, maybe it should be added as an assert to licenses itself?)
  (lib.groupBy
    (l: lib.toLower l.spdxId)
    (lib.filter (l: l ? spdxId)
      (lib.attrValues lib.licenses)))

# this PR
lib.attrsets.mapAttrs'
  (_key: license: {
    name = license.spdxId;
    value = license;
  })
  (lib.attrsets.filterAttrs (_key: license: license ? spdxId) lib.licenses)

Regardless, I'll rework this PR to what I mentioned, if anything just to experiment a bit! :)

@mibmo
Copy link
Contributor Author

mibmo commented Aug 14, 2024

@ShamrockLee check out this version :) I think this is significantly better.

Not sure if it's worth exposing licensesSpdxLowercase, but ... probably not!

@infinisil
Copy link
Member

Why the groupBy? when would there be duplicate licenses? (if this is a general concern, maybe it should be added as an assert to licenses itself?)

Nice, that's a good idea!

@ShamrockLee
Copy link
Contributor

Not sure if it's worth exposing licensesSpdxLowercase, but ... probably not!

This reminds me that we deliberately eliminate case differences during SPDX ID matching in getLicensesFromSpdxIdOr.

A function like getLicensesFromSpdxIdOr seems to be a more flexible and elegant solution than the exposure of spdxIdLowercase or something like that.

- Expose `lib.licensesSpdx`
- Create bindings for the needed internal functions
- Mention that some SPDX licenses might be missing (in the future I hope
  we can autogenerate the Nixpkgs license list from some SPDX endpoint
@infinisil
Copy link
Member

infinisil commented Aug 24, 2024

Tbh we shouldn't add symbols/functions when other functions already work, but in this case I do agree with both of you that just exposing the SPDX ID mapping would've been the better design from the start, so I think it's fine :)

I took the liberty to push some suggestions directly, I hope you don't mind! Let me know if they look good, then let's merge 🚀

@infinisil
Copy link
Member

infinisil commented Aug 24, 2024

A function like getLicensesFromSpdxIdOr seems to be a more flexible and elegant solution than the exposure of spdxIdLowercase or something like that.

Oh only just saw this now. I do wonder about that, because I'm now thinking that we could have a function like lib.attrsets.{get,has}AttrCaseInsensitive, which would be way more composable. The attribute set would have to be the first argument to enable it to be efficient though.

@infinisil
Copy link
Member

Why the groupBy? when would there be duplicate licenses? (if this is a general concern, maybe it should be added as an assert to licenses itself?)

Nice, that's a good idea!

More sidenotes: The list of licenses should ideally be automatically generated from the SPDX list directly somehow, and regularly updated, such that we don't even have to worry about that, and always have all the licenses available.

@mibmo
Copy link
Contributor Author

mibmo commented Aug 25, 2024

Oh only just saw this now. I do wonder about that, because I'm now thinking that we could have a function like lib.attrsets.{get,has}AttrCaseInsensitive, which would be way more composable. The attribute set would have to be the first argument to enable it to be efficient though.

I'm very much in favor of this, but wouldn't it almost be better to integrate it into Nix(CPP) itself? I'd venture to guess it fits the bill for a builtin.

  • Expose lib.licensesSpdx

oh, crap. Did I forgot to actually make it usable? .... either way, thanks for the cleanup! I don't know if there's anything left to do or discuss, other than perhaps for other people to review it, but I'll take the PR out of draft mode nonetheless. :)

@mibmo mibmo marked this pull request as ready for review August 25, 2024 13:02
@mibmo mibmo changed the title lib.licenses-spdx: mapping from SPDX ID to lib.licenses lib.meta.licensesSpdx: mapping from SPDX ID to compatible licenses Aug 25, 2024
@infinisil
Copy link
Member

I'm very much in favor of this, but wouldn't it almost be better to integrate it into Nix(CPP) itself? I'd venture to guess it fits the bill for a builtin.

I think in general we only need builtins if a non-builtin would be too inefficient, which I don't think is the case here :)

oh, crap. Did I forgot to actually make it usable?

Nah, it was just only usable as lib.meta.licensesSpdx before, but it's standard practice to expose most lib functions under lib.* as well

@infinisil infinisil merged commit 7954f9d into NixOS:master Aug 25, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants