Bug 2332598 - Review Request: rust-elasticlunr-rs - Generate static document search indexes
Summary: Review Request: rust-elasticlunr-rs - Generate static document search indexes
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: wojnilowicz
QA Contact: Fedora Extras Quality Assurance
URL: https://crates.io/crates/elasticlunr-rs
Whiteboard:
Depends On:
Blocks: 2332609
TreeView+ depends on / blocked
 
Reported: 2024-12-16 16:48 UTC by Fabio Valentini
Modified: 2025-03-18 22:48 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2025-03-18 22:48:38 UTC
Type: ---
Embargoed:
lukasz.wojnilowicz: fedora-review+


Attachments (Terms of Use)

Description Fabio Valentini 2024-12-16 16:48:12 UTC
Spec URL: https://decathorpe.fedorapeople.org/rust-elasticlunr-rs.spec
SRPM URL: https://decathorpe.fedorapeople.org/rust-elasticlunr-rs-3.0.2-1.fc41.src.rpm

Description:
A partial port of elasticlunr.js to Rust for generating static document
search indexes.

Fedora Account System Username: decathorpe

Comment 1 Fabio Valentini 2024-12-16 16:48:15 UTC
This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=126929929

Comment 2 Fabio Valentini 2024-12-16 16:48:37 UTC
Note: This is a re-review for a package that has been previously retired.

Comment 3 Fedora Review Service 2024-12-16 17:02:02 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8398300
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2332598-rust-elasticlunr-rs/fedora-rawhide-x86_64/08398300-rust-elasticlunr-rs/fedora-review/review.txt

Found issues:

- A package with this name already exists. Please check https://src.fedoraproject.org/rpms/rust-elasticlunr-rs
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 4 wojnilowicz 2025-02-13 19:40:55 UTC
Looks good to me except 
-license = "MIT/Apache-2.0"
+license = "(MIT OR Apache-2.0) AND MIT"
Did you add "AND MIT" due to the following files:
1) https://github.com/mattico/elasticlunr-rs/blob/master/LICENSE-JS
2) https://github.com/mattico/elasticlunr-rs/blob/master/LICENSE-WORDS
?

BTW, could you review https://bugzilla.redhat.com/show_bug.cgi?id=2344534 in return?

Comment 5 Fabio Valentini 2025-02-19 16:24:25 UTC
(In reply to wojnilowicz from comment #4)
> Looks good to me except 
> -license = "MIT/Apache-2.0"
> +license = "(MIT OR Apache-2.0) AND MIT"
> Did you add "AND MIT" due to the following files:
> 1) https://github.com/mattico/elasticlunr-rs/blob/master/LICENSE-JS
> 2) https://github.com/mattico/elasticlunr-rs/blob/master/LICENSE-WORDS
> ?

Yes, the README explicitly states that parts of the project are covered by different (i.e. only MIT) license:

> Includes code ported from [elasticlunr.js][eljs] Copyright (C) 2017 by Wei Song, 
> used under license. See LICENSE-JS for details.
>
> Includes stop word lists ported from [stopwords-filter][swft] Copyright (C) 2012 
> David J. Brenes, used under license. See LICENSE-WORDS for details.

Comment 6 wojnilowicz 2025-02-19 19:36:53 UTC
(In reply to Fabio Valentini from comment #5)
> (In reply to wojnilowicz from comment #4)
> > Looks good to me except 
> > -license = "MIT/Apache-2.0"
> > +license = "(MIT OR Apache-2.0) AND MIT"
> > Did you add "AND MIT" due to the following files:
> > 1) https://github.com/mattico/elasticlunr-rs/blob/master/LICENSE-JS
> > 2) https://github.com/mattico/elasticlunr-rs/blob/master/LICENSE-WORDS
> > ?
> 
> Yes, the README explicitly states that parts of the project are covered by
> different (i.e. only MIT) license:
> 
> > Includes code ported from [elasticlunr.js][eljs] Copyright (C) 2017 by Wei Song, 
> > used under license. See LICENSE-JS for details.
> >
> > Includes stop word lists ported from [stopwords-filter][swft] Copyright (C) 2012 
> > David J. Brenes, used under license. See LICENSE-WORDS for details.

Shouldn't that be reflected at this line https://github.com/mattico/elasticlunr-rs/blob/master/Cargo.toml#L3 ?

I mean shouldn't the following line
license = "MIT/Apache-2.0"
change to
license = "(MIT OR Apache-2.0) AND MIT"
to firstly remove ambiguity of the deprecated "/" char, and secondly truly reflect what the licensing terms are?
Details at https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-license-file-fields

Do you plan any PR for that?

Comment 7 Fabio Valentini 2025-03-15 23:12:13 UTC
Sorry for the delay in getting back to this.

> Shouldn't that be reflected at this line

For purposes of Fedora packaging, this is true.
But not everyone subscribes to the same school of thought on that topic as Red Hat Legal does.

I have filed an upstream issue to clarify this.
But for downstream packaging, the patch should already correctly reflect the project license(s).

https://github.com/mattico/elasticlunr-rs/issues/55

Comment 8 wojnilowicz 2025-03-16 07:03:18 UTC
(In reply to Fabio Valentini from comment #7)
> Sorry for the delay in getting back to this.
> 
> > Shouldn't that be reflected at this line
> 
> For purposes of Fedora packaging, this is true.
> But not everyone subscribes to the same school of thought on that topic as
> Red Hat Legal does.
> 
> I have filed an upstream issue to clarify this.
> But for downstream packaging, the patch should already correctly reflect the
> project license(s).
> 
> https://github.com/mattico/elasticlunr-rs/issues/55

No problem. At least you tried right now. Could you put that link near the license field in your SPEC file?
Other than that it LGTM.

BTW, I already fixed https://bugzilla.redhat.com/show_bug.cgi?id=2344534 Could you look at it as well?

===

Package was generated with rust2rpm (with some features removed due to missing dependencies), simplifying the review.

✅ package contains only permissible content
✅ package builds and installs without errors on rawhide
✅ test suite is run and all unit tests pass
✅ latest version of the crate is packaged
✅ license almost matches upstream specification and is acceptable for Fedora. Upstream misses MIT license and uses deprecated "/" specification but got notified about it.
✅ licenses of statically linked dependencies are correctly taken into account
✅ license file is included with %license in %files
✅ package complies with Rust Packaging Guidelines

Package APPROVED.

===

Recommended post-import rust-sig tasks:

- set up package on release-monitoring.org:
  project: $crate
  homepage: https://crates.io/crates/$crate
  backend: crates.io
  version scheme: semantic
  version (*NOT* pre-release) filter: alpha;beta;rc;pre
  distro: Fedora
  Package: rust-$crate

- add @rust-sig with "commit" access as package co-maintainer
  (should happen automatically)

- set bugzilla assignee overrides to @rust-sig (optional)

- track package in koschei for all built branches
  (should happen automatically once rust-sig is co-maintainer)

Comment 9 Fabio Valentini 2025-03-17 15:56:43 UTC
Thank you - upstream has already published a new version with the corrected license metadata, so I'll use that version and the patch and the spec file link become unnecessary :)

I'll take another look at awatcher ASAP.

Comment 10 Fabio Valentini 2025-03-18 22:48:38 UTC
Imported and built:
https://bodhi.fedoraproject.org/updates/FEDORA-2025-807ea167e1


Note You need to log in before you can comment on or make changes to this bug.