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

Fix license metadata #3233

Merged
merged 17 commits into from Apr 22, 2021
Merged

Fix license metadata #3233

merged 17 commits into from Apr 22, 2021

Conversation

bfsgr
Copy link
Contributor

@bfsgr bfsgr commented Mar 23, 2021

Adds license field in packages so that linux builds ship with the correct license.
Closes #3219, Closes INS-599.

@dimitropoulos
Copy link
Contributor

dimitropoulos commented Mar 24, 2021

While we're doing this, may I ask that we also change inso to use a valid license? Right now it's Apache2, which is close, but not on the list of valid licenses https://spdx.org/licenses/. We need to change it to Apache-2.0 (or, for that matter, MIT).

@nijikokun is that ok?

In case you think this is a pedantic request, some tooling reports this when you install, e.g.:

warning package.json: License should be a valid SPDX license expression

And, if nothing else, it'd be nice to have that error go away.

@nijikokun
Copy link
Contributor

Thanks for raising the issue @dimitropoulos. Let's go ahead and make sure it's properly labeled as Apache-2.0.

@bfsgr
Copy link
Contributor Author

bfsgr commented Mar 25, 2021

Thanks for raising the issue @dimitropoulos. Let's go ahead and make sure it's properly labeled as Apache-2.0.

Done 👍

@DMarby
Copy link
Contributor

DMarby commented Mar 25, 2021

I am not sure whether the changes in this PR will actually resolve #3219, as it is unclear whether actually having the license in the package.json will make electron-builder correctly set the license metadata as per the following open issues, or whether we need to include a debian/copyright file ourselves or similar:
electron-userland/electron-builder#4270
electron-userland/electron-builder#3914

That said, I still think we should merge the changes in this PR regardless, but I'm not sure if we can close #3219 until we verify the resulting .deb and if further changes are needed.

@bfsgr
Copy link
Contributor Author

bfsgr commented Mar 25, 2021

I tested it locally and putting the license field in package.json did create a correctly licensed .rpm, I didn't test it to .deb but I think it's safe to assume it will be correct as well.

Just to be safe I can test it again

@bfsgr
Copy link
Contributor Author

bfsgr commented Mar 25, 2021

@DMarby Just checked it locally and yes, both .rpm and .deb build with the correct license. However I'm not familiar with metadata to .AppImage and .snap but I can look into it too

@bfsgr
Copy link
Contributor Author

bfsgr commented Mar 27, 2021

Digging deeper I've found that the .snap does not ship with a license field inside meta/snap.yaml and I was unable to find metadata inside the .AppImage. I think the bigger issue may be with the .snap package.

But solving this maybe out of scope of this PR

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

I'll need to double check whether we should be applying MIT or Apache-2.0; to my knowledge it is the latter, so let's hold off on merging this until confirmed.

I echo the other points raised, however, that we should definitely get this in. Thank you for raising a PR and running through those investigations! 🤗

@vercel vercel bot temporarily deployed to Preview March 30, 2021 23:06 Inactive
@develohpanda develohpanda added N-investigation Needs: investigation S-blocked Status: Blocked labels Apr 12, 2021
@develohpanda
Copy link
Contributor

develohpanda commented Apr 12, 2021

A quick update, this is on the backlog to chase up with the right people. Until then, marking as blocked 🙂

@develohpanda develohpanda removed N-investigation Needs: investigation S-blocked Status: Blocked labels Apr 20, 2021
@develohpanda
Copy link
Contributor

Okay! I have some more information:

  • everything that doesn't have a license should go to Apache-2.0
  • everything that does have a license should stay as-is
  • set the root and insomnia-app licenses to MIT (we don't want to switch the license for the main app just yet)

@reynolek please advise if this doesn't capture it correctly 🙂

@reynolek
Copy link
Contributor

That looks correct from the info that I was given 👍

@bfsgr bfsgr changed the title Add MIT license into all packages missing it Fix license metadata Apr 21, 2021
@bfsgr
Copy link
Contributor Author

bfsgr commented Apr 21, 2021

Okay! I have some more information:

* everything that _doesn't_ have a license should go to `Apache-2.0`

* everything that _does_ have a license should stay as-is

* set the root and `insomnia-app` licenses to `MIT` (we don't want to switch the license for the main app just yet)

@reynolek please advise if this doesn't capture it correctly slightly_smiling_face

Alright, just applied the changes, please review

@reynolek reynolek added the C-improvement Category: Improvement / Enhancement label Apr 21, 2021
Copy link
Contributor

@reynolek reynolek left a comment

Choose a reason for hiding this comment

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

Approving from product perspective based on Legal team's input

@vercel vercel bot temporarily deployed to Preview April 22, 2021 20:00 Inactive
@develohpanda
Copy link
Contributor

Thank you for working through this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-improvement Category: Improvement / Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unknown license in releases
6 participants