Skip to content

Conversation

hsivonen
Copy link
Member

@hsivonen hsivonen commented Sep 3, 2025

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.

Following the docs didn't work on Ubuntu due to NSS integration failing to build, which is why I was unable to run the tests.

  • Tests: This PR includes thorough tests or an explanation of why it does not

This change is not supposed to change observable behavior.

  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

This is all about reducing dependencies (both Firefox and Application Services already depended on icu_normalizer, so this removes the duplicative dependency unicode-normalization) and about using dependencies that Firefox is trying to converge towards (icu_casemap is part of ICU4X despite Firefox not already having it vendored).

@hsivonen
Copy link
Member Author

hsivonen commented Sep 3, 2025

@hsivonen
Copy link
Member Author

hsivonen commented Sep 3, 2025

AFAICT, the cargo-audit failures are unrelated to this PR and also fail on main.

@hsivonen
Copy link
Member Author

hsivonen commented Sep 3, 2025

The other failures are real failures to remove the combining marks, though.

@hsivonen
Copy link
Member Author

hsivonen commented Sep 3, 2025

Looks like local debugging is needed to understand what's going wrong. is_combining_mark seems to work when testing on the Rust Playground.

@mhammond
Copy link
Member

mhammond commented Sep 3, 2025

I'll leave the review of the actual changes to the suggest team, but I think these new crates will block updating app-services in m-c until they are "audited" there - so we probably can't land this until they are.

@mhammond
Copy link
Member

mhammond commented Sep 3, 2025

hrm, I just noticed https://bugzilla.mozilla.org/show_bug.cgi?id=1986265, which is implying that some of these libs would already be in m-c. I'm not sure if it's just the rust implementations which are not, or that I'm simply wrong :) But I had a quick look at Cargo.lock in m-c and noticed some of the crates aren't already listed there, which implies an audit might be necessary.

@hsivonen
Copy link
Member Author

hsivonen commented Sep 3, 2025

icu_casemap and icu_casemap_data are not already in Firefox. The rest of the crates are.

I was expecting icu_casemap and icu_casemap_data to be audited as part of vendoring this to Firefox. I'm not sure if we have a workflow for auditing them ahead of time, but I'll look into it.

@hsivonen
Copy link
Member Author

hsivonen commented Sep 3, 2025

There isn't much to see in the audit delta, but I posted https://phabricator.services.mozilla.com/D263566 in case it helps the process along.

@hsivonen
Copy link
Member Author

hsivonen commented Sep 3, 2025

The iOS failure looks unrelated.

@hsivonen
Copy link
Member Author

hsivonen commented Sep 3, 2025

Looks like GitHub doesn't allow me to request review. CC @0c0w3

@mhammond
Copy link
Member

mhammond commented Sep 3, 2025

I was expecting icu_casemap and icu_casemap_data to be audited as part of vendoring this to Firefox. I'm not sure if we have a workflow for auditing them ahead of time, but I'll look into it.

Thanks! It should be fine to request review on just the audit bits and land them ahead of time - Nika has explicitly said this is OK and we've done that in the past for things like this

@@ -510,6 +510,7 @@ The following text applies to code linked from these dependencies:
[lalrpop-util](https://github.com/lalrpop/lalrpop),
[lazy_static](https://github.com/rust-lang-nursery/lazy-static.rs),
[libc](https://github.com/rust-lang/libc),
[linux-raw-sys](https://github.com/sunfishcode/linux-raw-sys),
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why this line wasn't here before. AFAICT, I don't add any new way to depend on this, and this dependency is pre-existing.

@0c0w3
Copy link
Contributor

0c0w3 commented Sep 3, 2025

I'm out of my depth on the unicode details but the main things I care about are that the suggest tests still pass and that merging this PR does not indefinitely block vendoring into m-c. We don't have any planned upcoming changes to suggest, so we don't have any plans to vendor in the near future, but I don't want to be blocked for a long time if something comes up. I don't think we have any planned changes to search either but I'm not as clear on that.

@hsivonen
Copy link
Member Author

hsivonen commented Sep 5, 2025

The icu_casemap audit has now landed: https://bugzilla.mozilla.org/show_bug.cgi?id=1986721

@hsivonen
Copy link
Member Author

hsivonen commented Sep 8, 2025

I'm out of my depth on the unicode details but the main things I care about are that the suggest tests still pass

The tests pass now.

and that merging this PR does not indefinitely block vendoring into m-c. We don't have any planned upcoming changes to suggest, so we don't have any plans to vendor in the near future, but I don't want to be blocked for a long time if something comes up.

It would be really nice to vendor this change to m-c sooner than later so that the vendoring script could be updated not to allow further dependencies on non-ICU4X crates that duplicate ICU4X functionality and, more importantly, data.

icu_casemap is now pre-audited, so vendoring this shouldn't require new audits.

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.

3 participants