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

Remove deprecated deny.toml fields #405

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

ammernico
Copy link
Member

@ammernico ammernico commented Aug 13, 2024

The fields were deprecated in version 0.14.12. The new default
behavior for the fields are:

  • unlicensed = "deny"
    New default: deny
  • copyleft = "deny"
    New default: deny
  • allow-osi-fsf-free = "either"
    New default: neither

It doesn't matter if the license is OSI and/or FSF free, only if it is
in the allow (or exception) list.

Link to the 0.14.12 changelog:
https://github.com/EmbarkStudios/cargo-deny/blob/main/CHANGELOG.md#01412---2024-02-23

This pull request describes the changes and new default behavior in
more detail:
EmbarkStudios/cargo-deny#611

This is a follow-up to #606 that actually provides a way to remove the
deprecated fields and opt in to the new behavior until the fields are
removed and the new behavior becomes the only behavior.

Since version 0.16.0 having these fields errors out.
https://github.com/EmbarkStudios/cargo-deny/blob/main/CHANGELOG.md#0160---2024-08-02

... making the usage of the deprecated fields into errors.

The diff in the comments are changes from the current cargo deny
template.

Copy link
Member

@primeos-work primeos-work left a comment

Choose a reason for hiding this comment

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

I don't like this approach - why use the full templates with all values, comments, etc.? I'd rather cleanup our current deny.toml first and only override the values we care about and just include a link to the upstream template/documentation instead.

Both approaches have advantages and drawbacks but it's likely more common to only set the settings one cares about.

@ammernico ammernico force-pushed the deny branch 2 times, most recently from d13a085 to 921daf3 Compare August 14, 2024 13:23
@ammernico ammernico requested a review from primeos-work August 14, 2024 13:29
@ammernico ammernico marked this pull request as ready for review August 14, 2024 13:30
Copy link
Member

@primeos-work primeos-work left a comment

Choose a reason for hiding this comment

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

The description of the changes needs a bit more work and I don't see version = 2 (EmbarkStudios/cargo-deny#611) anywhere so we're probably still using the old format version only with the deprecated fields removed.

I guess you can call this something like remove deprecated config fields and optionally add a commit to update to version two.

The commit message should link to an upstream changelog(/PR/etc.) to explain the background (e.g., https://github.com/EmbarkStudios/cargo-deny/blob/main/CHANGELOG.md#01412---2024-02-23 for when the fields got deprecated and https://github.com/EmbarkStudios/cargo-deny/blob/main/CHANGELOG.md#0160---2024-08-02 seems relevant as well).

Edit: https://embarkstudios.github.io/cargo-deny/index.html also seems useful.

@ammernico
Copy link
Member Author

Thanks for the feedback. I will add the version field and also update the version of the GitHub deny action.

I was also thinking about adding a link to the documentation. Decided against it in the end because I couldn't find a way to get a permanent URL to the current version/state of the documentation.
But I will include the other links

The fields were deprecated in version 0.14.12. The new default
behavior for the fields are:

- `unlicensed = "deny"`
  New default: `deny`
- `copyleft = "deny"`
  New default: `deny`
- `allow-osi-fsf-free = "either"`
  New default: `neither`
> It doesn't matter if the license is OSI and/or FSF free, only if it is
in the allow (or exception) list.

Link to the 0.14.12 changelog:
https://github.com/EmbarkStudios/cargo-deny/blob/main/CHANGELOG.md#01412---2024-02-23

This pull request describes the changes and new default behavior in
more detail:
EmbarkStudios/cargo-deny#611
> This is a follow-up to #606 that actually provides a way to remove the
deprecated fields and opt in to the new behavior until the fields are
removed and the new behavior becomes the only behavior.

Since version 0.16.0 having these fields errors out.
https://github.com/EmbarkStudios/cargo-deny/blob/main/CHANGELOG.md#0160---2024-08-02
> ... making the usage of the deprecated fields into errors.

The diff in the comments are changes from the current cargo deny
template.

Signed-off-by: Nico Steinle <nico.steinle@eviden.com>
@ammernico
Copy link
Member Author

There is no version bump for the deny GitHub action. No need to change it

Also found this in the documentation:
https://embarkstudios.github.io/cargo-deny/checks/licenses/cfg.html?highlight=version#the-version-field-optional

The version field is (at the time of this writing) no longer used, the following fields have been removed and will now emit errors.

Looks like they dropped the version field and went with their default behavior. We decided to omit it

@ammernico ammernico requested a review from primeos-work August 28, 2024 18:07
@ammernico ammernico changed the title Update the deny.toml to version two Remove deprecated deny.toml fields Aug 28, 2024
Copy link
Member

@primeos-work primeos-work left a comment

Choose a reason for hiding this comment

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

IANAL so I'll only approve this as a technical refactoring.

Comment on lines +6 to +12
"Apache-2.0",
"BSD-2-Clause",
"BSD-3-Clause",
"EPL-2.0",
"MIT",
"MPL-2.0",
"Unicode-DFS-2016"
Copy link
Member

Choose a reason for hiding this comment

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

LGTM. Unicode-DFS-2016 is only OSI approved, all other licenses are both FSF Free/Libre and OSI approved (https://spdx.org/licenses/).

Apparently the MPL-2.0 and EPL-2.0 are weak copyleft licenses. You only added the licenses to the list that are already required with the current dependencies but I thought it'd be a good idea to double-check this now. The MPL-2.0 uses "file-level" copyleft so that should be fine for us (https://www.mozilla.org/en-US/MPL/2.0/FAQ/). The EPL-2.0 only seems to feature some "limited"/weaker ("buisness-friendly") copyleft so it should be fine as well (https://en.wikipedia.org/wiki/Eclipse_Public_License).

cc @christophprokop

@primeos-work primeos-work added this pull request to the merge queue Sep 2, 2024
@primeos-work
Copy link
Member

There is no version bump for the deny GitHub action. No need to change it

Yes there is but it is optional and we can now use dependabot's PR: #414 :)

Merged via the queue into science-computing:master with commit d95972b Sep 2, 2024
13 checks passed
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.

2 participants