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

Add missing Paket license. Remove replaced bash dependencies generation script #127

Merged
merged 5 commits into from
Mar 19, 2019

Conversation

bpintea
Copy link
Collaborator

@bpintea bpintea commented Mar 13, 2019

This PR adds the missing Paket license info.
It also removes the bash dependencies generation script, now replaced with a batch equivalent (complementing #126).

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

I think it's called "Paket" (with no "c").

@droberts195
Copy link
Contributor

There are also the licenses of the components downloaded by Paket to consider. @codebrain said:

We can configure the paket.dependencies file such that it will download licenses as well: https://fsprojects.github.io/Paket/dependencies-file.html#License-download - it would then be possible to collect and bundle these files.

But then dependency_report.bat would need to include entries for those components into the single CSV file it produces for release manager. Maybe the code to do all this is then too complicated for a .bat file.

@bpintea bpintea changed the title Add missing Packet license. Remove replaced bash dependencies generation script Add missing Paket license. Remove replaced bash dependencies generation script Mar 13, 2019
@bpintea
Copy link
Collaborator Author

bpintea commented Mar 13, 2019

There are also the licenses of the components downloaded by Paket to consider.

There are and @codebrain and I had a quick discussion earlier on it.

Now, if the criteria for inclusion is based on repository and shipped product contents, I believe we only need to include Paket still.
If we need to list all build requirements - i.e. which software packages we use during the build process - we would need to add a longer list, not just those listed in https://github.com/elastic/open-source/issues/28#issuecomment-472263420 (at least CMake, maybe Gradle, Vagrant etc.?)

@droberts195
Copy link
Contributor

If we need to list all build requirements - i.e. which software packages we use during the build process

We don't need to list these in the CSV file for release manager. It only needs to list components that the end user who runs the installer will end up with on their system.

But that makes me wonder if Paket needs to be redistributed. If we are only using it to download components used during the build, do we really need to redistribute it? Given the permissive license it doesn't hurt to redistribute it. But if it's not being used to download any packages that the end user needs on their system it seems superfluous.

@bpintea
Copy link
Collaborator Author

bpintea commented Mar 13, 2019

But that makes me wonder if Paket needs to be redistributed. If we are only using it to download components used during the build, do we really need to redistribute it?

I'm trying to add it only to comply with https://github.com/elastic/open-source/issues/28:
"Third-party code contained in product repositories (including vendored code and copy/pasted code) is appropriately marked and exports NOTICE.txt and license information like other dependencies"

@droberts195
Copy link
Contributor

I think the key bit is:

like other dependencies

All the other stuff listed in (for example) https://artifacts.elastic.co/reports/dependencies/dependencies-6.6.2.html is stuff that end users will have on their systems if they install the relevant Elastic component. It doesn't include tools that are purely used during the build process.

It sounds like an equivalent component for Elasticsearch would be gradlew. That is committed to the elasticsearch repo and used during the build process, but not shipped as part of the distribution. And that does not appear in https://artifacts.elastic.co/reports/dependencies/dependencies-6.6.2.html.

The CSV file that release manager obtains by running the script in this repo will be used to create future pages like https://artifacts.elastic.co/reports/dependencies/dependencies-6.6.2.html. So I do not think the CSV file obtained from this repo during a release needs to include Paket as we do not redistribute it.

As for the license and notice files though I think you could be right that they should be somewhere in the repo. But maybe the installer/.paket directory would be a better place, to make clear that they relate to that tool we use internally rather than mixed with the licenses for the components we redistribute.

But to be honest I am probably the wrong person to be reviewing this PR. @tomcallahan and @legalastic what do you think?

@tomcallahan
Copy link

But that makes me wonder if Paket needs to be redistributed. If we are only using it to download components used during the build, do we really need to redistribute it?

I'm trying to add it only to comply with elastic/open-source#28:
"Third-party code contained in product repositories (including vendored code and copy/pasted code) is appropriately marked and exports NOTICE.txt and license information like other dependencies"

Based on the fact that it is build-time only and is MIT, we're OK and don't need to add it to a NOTICE.txt file assuming we haven't vendored any of its code.

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

Based on @tomcallahan’s feedback LGTM

@bpintea bpintea merged commit 63a148a into elastic:master Mar 19, 2019
@bpintea bpintea deleted the enh/packet_license branch March 19, 2019 21:02
bpintea added a commit that referenced this pull request Mar 19, 2019
…on script (#127)

* add Packet licensing details

* add version of used Packet

* removed the bash version of deps generation report

- this is replaced by the batch version now

* correct Paket spelling in license info

* s/Packet/Paket

(cherry picked from commit 63a148a)
bpintea added a commit that referenced this pull request Mar 19, 2019
…on script (#127)

* add Packet licensing details

* add version of used Packet

* removed the bash version of deps generation report

- this is replaced by the batch version now

* correct Paket spelling in license info

* s/Packet/Paket

(cherry picked from commit 63a148a)
bpintea added a commit that referenced this pull request Mar 19, 2019
…on script (#127)

* add Packet licensing details

* add version of used Packet

* removed the bash version of deps generation report

- this is replaced by the batch version now

* correct Paket spelling in license info

* s/Packet/Paket

(cherry picked from commit 63a148a)
bpintea added a commit that referenced this pull request Mar 19, 2019
…on script (#127)

* add Packet licensing details

* add version of used Packet

* removed the bash version of deps generation report

- this is replaced by the batch version now

* correct Paket spelling in license info

* s/Packet/Paket

(cherry picked from commit 63a148a)
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.

3 participants