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

rust: Add SPDX-License-Identifier and validate it in ci/codestyle.sh #2596

Merged
merged 2 commits into from
Feb 19, 2021

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Feb 18, 2021

It turns out we accidentally added GPL'd code into the Rust
side, which wasn't intentional on my part and I think it's since
been copied around.

Honestly I think half of the problem is the gigantic
"blah blah blah GNU General blah blah" just makes people's eyes
glaze over. In contrast the SPDX-License-Identifier is short
and obvious.

So let's validate that in CI.

This follows a similar change in ostree:
ostreedev/ostree#1439

If we merge this I'll do the C/C++ side too after that.

Avoid leaving cruft in the srcdir.
It turns out we accidentally added GPL'd code into the Rust
side, which wasn't intentional on my part and I think it's since
been copied around.

Honestly I think half of the problem is the gigantic
"blah blah blah GNU General blah blah" just makes people's eyes
glaze over.  In contrast the `SPDX-License-Identifier` is short
and obvious.

So let's validate that in CI.

This follows a similar change in ostree:
ostreedev/ostree#1439

If we merge this I'll do the C/C++ side too after that.
@cgwalters cgwalters changed the title Consistent spdx codestyle: Remove tabdamage.txt in OK case Avoid leaving cruft in the srcdir. --- Feb 18, 2021
@cgwalters cgwalters changed the title codestyle: Remove tabdamage.txt in OK case Avoid leaving cruft in the srcdir. --- rust: Add SPDX-License-Identifier and validate it in ci/codestyle.sh Feb 18, 2021
@lucab
Copy link
Contributor

lucab commented Feb 18, 2021

IMHO the source of the issue is that we have a boilerplate in every single file that basically nobody reads/checks, and it tends to accumulate copy-paste errors that way.

I'd rather get rid of that and just use the cargo field for the Rust side, and something like https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/ to cover the rest via wildcards.

@cgwalters
Copy link
Member Author

Hmm. I guess a guiding principle here is that we should avoid inventing our own system.

I personally really like SPDX-License-Identifier; the cost to using it is very low. In particular I think it's better than the debian/copyright approach which is just very fragile to code refactoring (modulo tools to validate it).

I'd also like to have a consistent story for our C/C++ and Rust code - particularly as we're actively moving the former to the latter, copyright needs to be obvious.

Another problem special to us in a degree we have is the mix of LGPL and GPL - I think that's what caused me to miss the GPL bits.

Other guidelines I've found:

@cgwalters
Copy link
Member Author

Also you're implicitly arguing we should also convert the current GPL-labeled Rust code to Apache-2.0 OR MIT - I think that would make sense but I think it should be a separate step after we've fixed our labeling and added CI.

@jlebon
Copy link
Member

jlebon commented Feb 18, 2021

/approve

Also you're implicitly arguing we should also convert the current GPL-labeled Rust code to Apache-2.0 OR MIT - I think that would make sense but I think it should be a separate step after we've fixed our labeling and added CI.

If we can relicense everything in Rust to the same license, I agree with Luca I'd be cleaner to just not have any header at all and just use the Cargo license field. This matches how most Rust projects do it.

For C/C++ files, I definitely prefer SPDX license headers to what we have now.

@cgwalters
Copy link
Member Author

This discussion relates of course to #2073 which turned into coreos/bootupd#149 which...would be nice to review too.

@lucab
Copy link
Contributor

lucab commented Feb 19, 2021

Also you're implicitly arguing we should also convert the current GPL-labeled Rust code to Apache-2.0 OR MIT - I think that would make sense but I think it should be a separate step after we've fixed our labeling and added CI.

Yes. As this is the actual issue at hand, I'm thinking it may make sense to uniform all the Rust licensing first, and then perform whatever project-wide changes.

I personally really like SPDX-License-Identifier; the cost to using it is very low. In particular I think it's better than the debian/copyright approach which is just very fragile to code refactoring (modulo tools to validate it).

Fair point. I do like @jlebon's suggested approach on this, although it isn't uniform across Rust and C.

@cgwalters
Copy link
Member Author

The problem is though we are actively translating C/C++ to Rust - and I don't want to have to also think about running git log on each file to validate we're OK converting the license too at the same time. We've been glossing over this a bit so far but I think that's wrong.

OTOH, I would like to try to keep at least some "library-ish" portions of our Rust dual ASL2.0/MIT for the stated reason of making them easier to split out as crates to crates.io.

This to me is an unusual situation that makes our Rust code not like other Rust code, and hence my argument for using SPDX-License-Identifier across the board.

@cgwalters
Copy link
Member Author

And to clarify I think the license field in the Cargo metadata needs then to be at least LGPLv2-or-later and probably to start GPLv2-or-later, even if some of the individual files are still dual ASL2.0/MIT.

@cgwalters
Copy link
Member Author

cgwalters commented Feb 19, 2021

To briefly recap the history here:

  • ostree is LGPLv2+ because that's the GNOME standard for libraries; and also notably systemd standardized on LGPLv2+
  • rpm-ostree is created as also LGPLv2+ because it makes it easier to share code back and forth
  • Rust language appears on the scene, creates its own ecosystem standard of ASL2.0/MIT
  • rpm-ostree daemon code is created (as GPL)
  • At some point I noticed that (IMO way too easy to overlook) GPL-vs-LGPL distinction in the source files: incorrect GPL headers #1890 - I'm still not sure if it was intentional but the author of the code is no longer at Red Hat, and honestly I didn't have the energy to try to actively re-convert to LGPLv2+
  • Rust code appeared and for the same "code sharing with ecosystem" reasons we try to make it ASL2.0/MIT

Now we're blending those two worlds actively.

@jlebon
Copy link
Member

jlebon commented Feb 19, 2021

The problem is though we are actively translating C/C++ to Rust - and I don't want to have to also think about running git log on each file to validate we're OK converting the license too at the same time. We've been glossing over this a bit so far but I think that's wrong.

Hmm, didn't realize this. So all the code we rewrite has to essentially retain the same license from the C code? So that means that even after we've fully converted everything to Rust, we'll still have a mix of Apache-2.0/MIT and LGPL (and GPL) ?

Anyway, this is a good cleanup, and the CI check is good, so
/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cgwalters
Copy link
Member Author

So all the code we rewrite has to essentially retain the same license from the C code?

IANAL but basically as I understand it, if you want to change the license, it has to be new code that you wrote without looking at the old code. For example, when going to implement some APIs in Rust all you do is look at the C/C++ header files and write a Rust equivalent, that would be considered new code. But if you look at the implementation, and particularly you're doing anything like a "literal translation" - you're creating a derived work covered by the original copyright.

This issue is best exemplified by https://en.wikipedia.org/wiki/Google_LLC_v._Oracle_America,_Inc. which is all about whether Google reimplementing parts of the JDK by simply looking at their APIs means they're infringing Oracle copyright. (And really everyone in software needs to root for Google on this one because otherwise...so many APIs would be copyrighted and it'd be lawsuits flying everywhere)

@openshift-merge-robot openshift-merge-robot merged commit f882d6d into coreos:master Feb 19, 2021
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.

5 participants