puma / puma

A Ruby/Rack web server built for parallelism
https://puma.io
BSD 3-Clause "New" or "Revised" License
7.62k stars 1.42k forks source link

Specify multiple licences in the gemspec? #3311

Open voxik opened 5 months ago

voxik commented 5 months ago

The sd_notify is bundled in Puma since #3011. If it is hard dependency, then I believe that specifying such dependency in .gemspec would be much better option:

$ git diff
diff --git a/puma.gemspec b/puma.gemspec
index f5e01c25..a153364a 100644
--- a/puma.gemspec
+++ b/puma.gemspec
@@ -29,4 +29,6 @@ Gem::Specification.new do |s|

   s.license = "BSD-3-Clause"
   s.required_ruby_version = Gem::Requirement.new(">= 2.4")
+
+  s.add_runtime_dependency 'sd_notify', '~> 0.1'
 end

Should you insist that this is the best option, could you please fix the license field to correctly reflect that the code is not just BSD-3-Clause licensed? I'd be also more then happy if the code was in e.g. vendor subdirectory, to make this clear.

voxik commented 5 months ago

BTW it would also be nice, if it was clear what version of the file is bundled and if there was clear path to update the file if needed.

dentarg commented 5 months ago

sd_notify is an optional dependency. It will not be forced upon every user of Puma.

dentarg commented 5 months ago

Let's discuss your comment at https://github.com/puma/puma/pull/3011#issuecomment-1883329515 here instead of the closed PR

As a Fedora maintainer, I am disappointed with this PR and with bundling in general. What was the reason to not specify the dependency in .gemspec? If at least the code was put into dedicated directory to make this more obvious. And if at least the license field was updated to list the MIT license alongside the BSD-3-Clause.

Instead of expressing your disappointment, can you explain the issues you are facing? What have gone wrong in your opinion here? Something with licenses?

I think the reason for vendoring the dependency has been stated in #3011 (same as Sidekiq)

dentarg commented 5 months ago

sd_notify is an optional dependency. It will not be forced upon every user of Puma

I guess that wasn't the most clever statement, as I realise it can be seen that it as been forced upon every user of Puma via the vendoring, but what I meant to express was that it will probably not ever end up in the gemspec as Puma really really want to have as few runtime dependencies via bundler as possible, due to certain (restart/reload) features.

voxik commented 5 months ago

Fedora is strongly against bundling:

https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling

And from that POV, having sd_notify as an (mandatory) external dependency would provide to Puma users the same service. Not sure what are the "certain (restart/reload) features", which would speak for bundling, but they were not elaborated in the initial PR.

The Sidekiq discussion says "I didn't want to require the user to add the gem to their gemfile manually", which would not be the case if the dependency was properly specified in the .gemspec. The "~50 lines of stable code" is not argument at all. On the Fedora level, we can hardly treat 50 lines different than e.g. 100 lines. But I have not tried to read through the whole discussion so maybe there is more.

Something with licenses?

Yes, this is the most pressing issue. The gem metadata should be trustworthy source of license information IMHO. But they were not updated.

I think that e.g. Readme or some other file which is easily discoverable should discuss the license.

If there are still reasons for bundling, it should at least "stick out".

dentarg commented 5 months ago

Not sure what are the "certain (restart/reload) features", which would speak for bundling

I was thinking of phased restart: https://github.com/puma/puma/discussions/3034

voxik commented 5 months ago

Not sure what are the "certain (restart/reload) features", which would speak for bundling

I was thinking of phased restart: #3034

Thx for pointing this out. That was not clear from the PR. I am going to change the subject to something hopefully acceptable :blush:

dentarg commented 5 months ago

Even if Puma didn't have the phased restart feature, I suspect we wouldn't want to add an explicit dependency on the sd_notify gem, as it is (my guess) used by a minority of users. It would not be worth the addition to everyones Gemfile and any potential future incompatibilities (resolution wise). So then vendoring was proposed and accepted. It seems to be a common thing for this piece of code, here's another example: https://github.com/bensheldon/good_job/pull/1029

I see you change the title here to Please make sure sd_notify if properly bundled @voxik but I'm still not sure what you're suggesting?

dentarg commented 5 months ago

Looks like it possible to specify multiple licences in the gemspec: https://guides.rubygems.org/specification-reference/#licenses=

voxik commented 5 months ago

but I'm still not sure what you're suggesting?

Fix the license situation at minimum. I.e. add the MIT into .gemspec and elaborate about the licenses in general somewhere. Either in something such as LICENSES file or in the Readme.

If the bundled code was placed in e.g. "vendor" directory, to make it obvious that the code does not originate from this project, that would not hurt.

voxik commented 5 months ago

And it would actually be also helpful if the steps to update the sd_notify were documented

dentarg commented 5 months ago

Feel free to contribute :)

nateberkopec commented 5 months ago

I'm going to remove contrib-wanted here because I don't agree with the action step and would like to discuss further.

I don't like the option of updating license or licenses in the gemspec, because those fields are generally used to describe the entire project. The licenses field exists for the quite common scenario of dual or multi-licensing.

What I do not want is for someone to think BSD, MIT in the license field applies to the entire Puma project, as that is not how this project is licensed. We're talking about 50 lines of code in a 5000+ line library, giving the two equal billing in the licenses field doesn't describe what's happening here. The docs for license= specify it should only contain license names, not an explanation such as "BSD 3-clause, some parts MIT".

I see OP's problem as: "I, as an OS package maintainer, need to be able to easily identify subpackages (bundled or not), along with their licensing and versioning."

We could change the License section of the Readme to something like:

Puma is copyright Evan Phoenix and contributors, licensed under the BSD 3-Clause license. See the included LICENSE file for details.

_Puma contains a distribution of sd_notify v0.1.1, a Rubygem licensed under the MIT license. See lib/puma/sdnotify.rb for more details._

WDYT?

MSP-Greg commented 5 months ago

@nateberkopec

I'd agree, but I'm also not familiar with automated licensing tools, so I'm not much help. Not sure about the differences between the MIT and 'BSD 3-Clause' licenses. Not an IP lawyer...

nateberkopec commented 5 months ago

@voxik I notice you had a similar issue here and it looks like that was resolved for you by including a LICENSE.txt file alongside the vendored code (rather than in the vendored file, like we did). Is that another option?

nateberkopec commented 5 months ago

As an aside, I don't think the gemspec can be the solution because a Rubygem can bundle code that isn't even Ruby (like, a Javascript library or something for example). Another gem that I maintain actually does this. The gemspec can only specify Ruby dependencies.

voxik commented 5 months ago

We could change the License section of the Readme to something like:

Puma is copyright Evan Phoenix and contributors, licensed under the BSD 3-Clause license. See the included LICENSE file for details.

_Puma contains a distribution of sd_notify v0.1.1, a Rubygem licensed under the MIT license. See lib/puma/sdnotify.rb for more details._

This certainly good step which I would appreciate.

@voxik I notice you had a similar issue here and it looks like that was resolved for you by including a LICENSE.txt file alongside the vendored code (rather than in the vendored file, like we did). Is that another option?

If you are referring to a concept of some license file, then yes. But otherwise, this was a bit different situation because RubyGems itself is not shipped as a .gem and therefore does not have .gemspec metadata, which would provided some condensed information about the project.

What I do not want is for someone to think BSD, MIT in the license field applies to the entire Puma project, as that is not how this project is licensed. We're talking about 50 lines of code in a 5000+ line library, giving the two equal billing in the licenses field doesn't describe what's happening here.

I don't think that amount of lines or what not is metrics which could be used to judge this (and there are another discussions elsewhere about effective license, etc). However I understand your concern and unfortunately, I have yet to see any place where some license information would be as detailed as "99 % BSD, 1 % MIT". Unfortunately, the Gem metadata are even less verbose. I have recently opened this ticket against RubyGems, but I am afraid it would helped just a bit (and complicated another matters).

As an aside, I don't think the gemspec can be the solution because a Rubygem can bundle code that isn't even Ruby (like, a Javascript library or something for example). Another gem that I maintain actually does this. The gemspec can only specify Ruby dependencies.

Maybe I don't understand, but if the Javascript is distributed together with the Ruby code as part of the .gem file, then the license specified in the .gemspec applies IMHO.

Nevertheless, since there were reference to what is the meaning of the .gemspec license field, there is one other option described by the documentation:

You can also use a custom license file along with your gemspec and specify a LicenseRef-<idstring>, where idstring is the name of the file containing the license text.

The downside is that the short identifiers won't be visible, OTOH, it allows to refer to the file, where the license situation can be elaborated in detail.

As an aside, overseeing around ~500 gems in Fedora, I have yet to see the LicenseRef used in practice. And maybe it is obsolete practice and DocumentRef (which is not supported by RubyGems AFAICT) should be used instead.