Skip to content

Bump ahash dependency version#770

Closed
joshlf wants to merge 1 commit intoamethyst:masterfrom
joshlf:patch-1
Closed

Bump ahash dependency version#770
joshlf wants to merge 1 commit intoamethyst:masterfrom
joshlf:patch-1

Conversation

@joshlf
Copy link
Copy Markdown

@joshlf joshlf commented Oct 23, 2023

The previous version had a bug, and was yanked.

Checklist

  • I've added tests for all code changes and additions (where applicable)
  • I've added a demonstration of the new feature to one or more examples
  • I've updated the book to reflect my changes
  • Usage of new public items is shown in the API docs

API changes

The previous version had a bug, and was yanked.
@juliusl
Copy link
Copy Markdown

juliusl commented Oct 23, 2023

track #769

@juliusl
Copy link
Copy Markdown

juliusl commented Oct 23, 2023

@joshlf shred needs to be bumped too

@joshlf
Copy link
Copy Markdown
Author

joshlf commented Oct 23, 2023

@joshlf shred needs to be bumped too

To 0.15.1?

@joshlf
Copy link
Copy Markdown
Author

joshlf commented Oct 23, 2023

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: amethyst/shred#231

@zesterer
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Author

joshlf commented Oct 24, 2023

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

@zesterer
Copy link
Copy Markdown
Contributor

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

@Imberflur
Copy link
Copy Markdown
Contributor

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

@joshlf
Copy link
Copy Markdown
Author

joshlf commented Oct 24, 2023

@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
Copy link
Copy Markdown

juliusl commented Oct 24, 2023

@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
Copy link
Copy Markdown
Author

joshlf commented Oct 24, 2023

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

@xMAC94x xMAC94x mentioned this pull request May 13, 2024
4 tasks
@Imberflur
Copy link
Copy Markdown
Contributor

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)

@Imberflur Imberflur closed this May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants