-
Notifications
You must be signed in to change notification settings - Fork 47
Fetching of the license file from relative path #3534
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
base: main
Are you sure you want to change the base?
Conversation
|
I'm not sure this actually fixes #3477 in a desirable way.
|
I think some of the points in the License do apply to the binary distribution, so the click-through should be there.
This is a windows specific fix, as brought up in #3477
Yes, the file should be removed. |
jamescrake-merani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@llimeht I'm not sure I agree. We have the licence in the documentation but there's no harm on having it in the installer too. It seems Git on Windows also shows the GPL, so I don't think this is unusual.
I've verified this works on my Windows desktop so I'm approving. You may want to remove the licence file in this PR before merging, or in a separate one.
Also I noticed that some files have this on the header:
"""
This software was developed by the University of Tennessee as part of the
Distributed Data Analysis of Neutron Scattering Experiments (DANSE)
project funded by the US National Science Foundation.
See the license text in license.txt
copyright 2010, University of Tennessee
We should probably update this. But also, I'm not even sure if its necessary to keep this header at all. But that's probably a separate discussion to have.
|
It's permitted to show the licence but it is absolutely not required. The only thing that is required is that the binary distribution have a copy of the licence which is what §2 of the licence says. It's a happily settled piece of free software legalese that the user is not required to click-through the licence; indeed the user is actually not even required to accept the licence as the licence controls distribution not usage. (and note that sasview wasn't even carrying a copy of its own licence... in violation of its own licence... until #3480) I still think 2 changes are required here:
|
Should we then add this to the main LICENSE.TXT? But, we already have the acknowledgement request with proper wording (more text) in the |
Adding it as a request outside of the legal licence text is ok. Adding it to the actual licence text is a hard "no". (a) we absolutely should not try inventing new licences (I've seen this enough times, it always ends badly), and (b) a licence change would need legal sign-off from every single copyright holder (either the individual or the institution, depending on their circumstances). I think it's also worth taking a step back on this whole PR. What actual problem are we trying to fix here? I don't believe there's actually a bug to fix (and if there is one, then it needs to be fixed everywhere, not only in the windows installer, and that means fixing it in the UI) |
|
The point is to have consistent licensing text in both the installer display window and inside the app. I would propose to merge this as-is, since it fixes the obvious issue of having different license wording between the two locations. Acknowledgements can be resolved at some other point. |
If the installer displayed a difference licence to what sasview was actually released under then that would be an issue. However, that's not the case. The installer does not currently display any licence text - it's not that it's inconsistent, it doesn't do it at all, and that's ok.
No, this PR creates the issue with the acknowledgement text, while not fixing a problem that I really don't think exists.
No, as an alternative, we could not merge this PR, and then that wouldn't be needed.
There isn't different licence text in two locations. There is the licence text in the source & documentation, and a request for acknowledgement in the Windows installer. That's self-consistent and OK, while this PR creates problems that then need to be fixed. If we really wanted we could add the licence text to that request for acknowledgement, (but it's not needed, it's just clutter). The only inconsistency is that the file in git is called |
|
Reading through this PR, I think @llimeht makes some very good points on licensing and what is displayed on install. I would suggest this be part of the discussion for the biweekly call to be sure this is what we want to do. |
|
As there hasn't been an agreement on this PR yet, I am not going to merge it for 6.1.1. We will need to have further discussions about this post-release. |
|
This was discussed at today's fortnightly meeting. This remains as an approved PR though supposedly discussion was ongoing up till August? However nobody on the call could remember the issues. There was also some suggestion that the original concern of the license not being in the bundle was resolved elsewhere already? Based on this no action was taken. However it is noted that we should finalize that discussion so that PRs don't get left hanging indefinitely. |
|
@butlerpd, This PR is a request to change the way SasView presents itself to new users. It is deleting the request for acknowledgements from the installer and instead showing the licence text. We might very well want to make that swap (I doubt it) but that is something to be discussed as a proposed change in policy (for want of a better word), not done by accident in a PR. There is no technical or legal bug being fixed by this PR. If having the request for acks in a file called "license.txt" is problematic then we can do llimeht@9727234 In answer to your approved-but-not-merged observation, if there were a "disapprove" button I'd use it here, but fortunately there is not :) |
Description
Added relative path to the correct license file to be used in the Windows installer.
Fixes #3477
How Has This Been Tested?
Local test on GH built installer.
Review Checklist:
[if using the editor, use
[x]in place of[ ]to check a box]Documentation (check at least one)
Installers
Licensing (untick if necessary)