amethyst / specs

Specs - Parallel ECS
https://amethyst.github.io/specs/
Apache License 2.0
2.49k stars 219 forks source link

Bump ahash dependency version #770

Closed joshlf closed 4 months ago

joshlf commented 10 months ago

The previous version had a bug, and was yanked.

Checklist

API changes

juliusl commented 10 months ago

track #769

juliusl commented 10 months ago

@joshlf shred needs to be bumped too

joshlf commented 10 months ago

@joshlf shred needs to be bumped too

To 0.15.1?

joshlf commented 10 months ago

Oh sorry, do you mean that I should open a separate PR to the shred repo? https://github.com/amethyst/shred

If so, that's here: https://github.com/amethyst/shred/pull/231

zesterer commented 10 months ago

Thanks all. It's unfortunate that this is required since the original issue in question isn't of particular concern to specs, but ah well.

zesterer commented 10 months ago

Thanks all. It's unfortunate that this is required since the original issue in question isn't of particular concern to specs, but ah well.

The licensing issue caught by CI is a problem, however. I'd argue the fault here lies with ahash since it's pulling in a dependency (zerocopy) that is not compatible with its own license.

joshlf commented 10 months ago

Would it be an acceptable solution to add BSD-2-Clause as an allowed license? BSD-3-Clause is already allowed.

zesterer commented 10 months ago

It might be, yes. @kvark @Imberflur Do either of you have thoughts on this?

Imberflur commented 10 months ago

@zesterer

cargo deny CI was added in https://github.com/amethyst/specs/pull/749 and there was a bit discussion on it here https://github.com/amethyst/specs/pull/749#discussion_r903036768. I think the current set of allows matches what was needed for the dependencies at the time.

I'm not familiar with what kind of licensing effect would arise from allowing BSD-2-Clause and I don't have any particular opinions here.

It is possible to add exceptions for specific crates (rather than a blanket allow for a license), such that taking on new dependencies with that license requires a conscious decision: https://embarkstudios.github.io/cargo-deny/checks/licenses/cfg.html#the-allow-field

We could potentially allow this for now and if having BSD-2-Clause deps ends up being an issue later, find a path to remove them?

Also a 0.7.7 version of ahash was published so this PR should not be essential for dealing with the other versions being yanked. Ofc, it is nice to update to the latest version regardless.

joshlf commented 10 months ago

@zesterer

cargo deny CI was added in #749 and there was a bit discussion on it here #749 (comment). I think the current set of allows matches what was needed for the dependencies at the time.

I'm not familiar with what kind of licensing effect would arise from allowing BSD-2-Clause and I don't have any particular opinions here.

It is possible to add exceptions for specific crates (rather than a blanket allow for a license), such that taking on new dependencies with that license requires a conscious decision: https://embarkstudios.github.io/cargo-deny/checks/licenses/cfg.html#the-allow-field

We could potentially allow this for now and if having BSD-2-Clause deps ends up being an issue later, find a path to remove them?

Also a 0.7.7 version of ahash was published so this PR should not be essential for dealing with the other versions being yanked. Ofc, it is nice to update to the latest version regardless.

I found this on the difference between the licenses:

The Modified or New BSD (or BSD 3-clause) license is the same as BSD-2, but with an additional clause prohibiting the names of the authors from being used to endorse or promote products relating to the software.

Indeed, using the text of the 2- and 3-clause variants from here and here, this appears to be an accurate description:

$ diff 2_CLAUSE 3_CLAUSE 
1c1
< BSD 2-Clause License
---
> BSD 3-Clause License
13a14,17
> 
> 3. Neither the name of the copyright holder nor the names of its
>    contributors may be used to endorse or promote products derived from
>    this software without specific prior written permission.

IANAL, but I'd assume that if your project is already comfortable with BSD-3-Clause, then the same logic would apply for being comfortable with a license which imposes strictly fewer restrictions on your project.

juliusl commented 10 months ago

@joshlf sorry for the late reply, To clarify I'm just pointing it out to note that shred needs to be updated and released before this can be merged. Ran into it when I while I was patching for this in some of my other repos.

joshlf commented 10 months ago

@joshlf sorry for the late reply, To clarify I'm just pointing it out to note that shred needs to be updated and released before this can be merged. Ran into it when I while I was patching for this in some of my other repos.

Oh gotcha, makes sense!

Imberflur commented 4 months ago

Superceded by #781. Sorry for the delay here, and thanks for resolving the licensing issue! (even if BSD-2-Clause might have been fine since BSD-3-Clause is already allowed)