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: Added notice for third party licenses #11483

Merged
merged 15 commits into from
Mar 1, 2023

Conversation

hishitetsu
Copy link
Member

@hishitetsu hishitetsu commented Feb 27, 2023

Since Files contains third-party software that is not under the MIT license, it should be clearly stated that a different license applies to them.
I also think it should be added to third party licenses display on the app's settings screen, but first I submit a pull request for the license file.

Edit:
Now LICENSE is unchanged. I have added NOTICE.md to clarify third-party software licenses.

LICENSE Outdated Show resolved Hide resolved
@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

This is invalid Licensing. Please refer to GPL license. If Files has ANY code licensed under the GPL, whole code must be licensed under the GPL. And so, we have to remove code licensed under the GPL(especially FilesLauncher).

This kind declaration is only valid for 'Attribution' such as CC license.

@hishitetsu
Copy link
Member Author

If Files has ANY code licensed under the GPL, whole code must be licensed under the GPL.

This is true if Files links to GPL codes. However, the GPL code is only used for FilesLauncher. Files itself uses no GPL codes, just includes FilesLancher.exe.
So I don't think it is a clear violation of the GPL.

In summary, I think Files can be described as a collection of the following four pieces of software.

  • LGPL licensed 7z.dll
  • LGPL licensed SevenZipSharp
  • GPL licensed FilesLauncher.exe
  • All remaining codes which is MIT licensed and can be copyrighted

@yaira2
Copy link
Member

yaira2 commented Feb 27, 2023

FilesLauncher

Most of FilesLauncher is custom built by @gave92, we should look into rewriting the GPL parts.

@hishitetsu
Copy link
Member Author

I have made a few changes. Other than FilesLauncher, this repository also contains LGPL binaries, so it would be bad to simply show only the MIT license.

@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

Let me be clear about Inability to coexist

Excerpt from GPL v3, Section 5, Paragraph c:

  5. Conveying Modified Source Versions.

    c) You must license the entire work, as a whole, under this
    License to anyone who comes into possession of a copy.  This
    License will therefore apply, along with any applicable section 7
    additional terms, to the whole of the work, and all its parts,
    regardless of how they are packaged.  This License gives no
    permission to license the work in any other way, but it does not
    invalidate such permission if you have separately received it.

@hishitetsu
Copy link
Member Author

I think "the entire work" here can be limited to FileLauncher since it is an independent program.
This would be the case shown in the following FAQ.
https://www.gnu.org/licenses/gpl-faq.html.en#GPLCompatInstaller

@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

A lot of articles says 'you must have entire work be under the GPL'. Some of, don't have to. Quite confused...

should write those statements in NOTICE.md

https://github.com/microsoft/terminal/blob/main/NOTICE.md

@hishitetsu
Copy link
Member Author

hishitetsu commented Feb 27, 2023

A lot of articles says 'you must have entire work be under the GPL'.

If so, linux distributions would have to distribute all software under the GPL.
It depends on how we view this FAQ. I think they can be considered two separate programs, not one program with two parts.
https://www.gnu.org/licenses/gpl-faq.html.en#MereAggregation

@hishitetsu
Copy link
Member Author

should write those statements in NOTICE.md

This is a good idea. Once we have a policy in place, I will add NOTICE.md instead of updating LICENSE.

@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

I finally found that MIT and GPL have compatibility and can be coexisted.
I'm wanna talk about that it is ok if we can wrap source codes including GPL License with MIT License in the first place. I think we cannot, so anyway we probably have to remove FilesLauncher.

@hishitetsu
Copy link
Member Author

No. So, ideally, it would be better to move the GPLed code to a separate repository. But It would be okay to just specify which code is MIT-licensed and which is GPL-licensed.

@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

Where do you think we state that? LICENSE file cannot be changed cuz it's template. We probably have to add two-line-license-acknowledgement to all of source code like Microsoft's OSS?

Copyright (c) 2023 Files
Licensed under the MIT License. See the LICENSE file.

@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

which file was the one with GPL code? Are we even using it?

FilesLauncher. One reference in Files.App

private bool DetectIsSetAsDefaultFileManager()
{
using var subkey = Registry.ClassesRoot.OpenSubKey(@"Folder\shell\open\command");
var command = (string?)subkey?.GetValue(string.Empty);
return !string.IsNullOrEmpty(command) && command.Contains("FilesLauncher.exe");
}

@hishitetsu
Copy link
Member Author

FilesLauncher is used to replace File Explorer. It was introduced in #8684.

@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

The files which have a statement:

// Copyright (C) Explorer++ Project
// SPDX-License-Identifier: GPL-3.0-only
// See LICENSE in the top level directory

There's no license that include this license, though.

  • DocumentServiceProvider.cpp
  • DocumentServiceProvider.h
  • ServiceProviderBase.cpp
  • ServiceProviderBase.h
  • ShellView.cpp
  • ShellView.h
  • WebBrowserApp.cpp
  • WebBrowserApp.h

@hishitetsu
Copy link
Member Author

We could revert #8684, but that would bring back the problem that was solved by #8684.

@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

We cannot revert. Only we can do is to rewrite/remove those codes in C# or C++. @gave92 created those files so we may ask him if those codes can be replaced/removed with our own C# or C++.

@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

We should open an issue. (Thank you for bringing this discussion)

@yaira2
Copy link
Member

yaira2 commented Feb 27, 2023

The files which have a statement:

// Copyright (C) Explorer++ Project
// SPDX-License-Identifier: GPL-3.0-only
// See LICENSE in the top level directory

There's no license that include this license, though.

  • DocumentServiceProvider.cpp
  • DocumentServiceProvider.h
  • ServiceProviderBase.cpp
  • ServiceProviderBase.h
  • ShellView.cpp
  • ShellView.h
  • WebBrowserApp.cpp
  • WebBrowserApp.h

I don't believe any of these are actively being used for setting the default file manager, these are for the open file picker which is still a very early work in progress. I think they can be safely removed.

@hishitetsu hishitetsu marked this pull request as draft February 27, 2023 04:33
@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

@yaira2 Can I work on?

@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

@hishitetsu My suggestion

  • Revert all changes in LICENSE.
  • Add NOTICE.md following format

# THIRT PARTY NOTICE and ACKNOWLEDGMENTS

**Note: DO NOT localize.**

## 7Zip

**Source code:** https://example.com

### License

\```
\```

THIRT PARTY NOTICE and ACKNOWLEDGMENTS

Note: DO NOT localize.

7Zip

Source code: https://example.com

License

@hishitetsu
Copy link
Member Author

Yes, I will do so.
How about FilesLauncher? Should we include the mention of FilesLauncher in NOTICE.md?

@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

I don't think so. I will immediately delete them at least until this weekend.

@yaira2
Copy link
Member

yaira2 commented Feb 27, 2023

Add NOTICE.md following format

Is it enough to have this in the about page?

@0x5bfa
Copy link
Member

0x5bfa commented Feb 27, 2023

It'd prefer both.

@hishitetsu
Copy link
Member Author

For the licenses dialog, we could reuse some code from SecureFolderFS

Thanks for the information!
I mistakenly thought that SecureFolderFS also needs to be added to NOTICE. In any case, I have added a note about any mistakes in NOTICE.

@hishitetsu hishitetsu changed the title Add third party notices Added third party notices Feb 27, 2023
@hishitetsu
Copy link
Member Author

hishitetsu commented Feb 28, 2023

I intend to display NOTICE.md on the about page like this.
image
Though, we need to decide on the contents of NOTICE.md first.

@yaira2
Copy link
Member

yaira2 commented Feb 28, 2023

I believe @d2dyno1 is working on the one for the about page.

@hishitetsu
Copy link
Member Author

Really? I worked on it too...

@yaira2
Copy link
Member

yaira2 commented Feb 28, 2023

The design @d2dyno1 is working on will have a separate page for licenses so I think we should wait for it. In the meantime, is this PR ready?

@hishitetsu
Copy link
Member Author

hishitetsu commented Feb 28, 2023

In the meantime, is this PR ready?

Yes. Unless pointed out by others.

@yaira2 yaira2 requested a review from d2dyno1 February 28, 2023 02:09
0x5bfa
0x5bfa previously approved these changes Feb 28, 2023
Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

LGTM.

@yaira2 yaira2 changed the title Added third party notices Fix: Added notice for third party licenses Mar 1, 2023
NOTICE.md Outdated Show resolved Hide resolved
NOTICE.md Outdated Show resolved Hide resolved
Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Mar 1, 2023
Copy link
Member

@yaira2 yaira2 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

@yaira2 yaira2 merged commit 758f61d into files-community:main Mar 1, 2023
@yaira2 yaira2 changed the title Fix: Added notice for third party licenses Fix: Added notice for third party licenses Mar 1, 2023
@hishitetsu hishitetsu deleted the UpdateLicense branch March 1, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants