Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync with 10.0.1 #134

Merged
merged 29 commits into from
Oct 1, 2024
Merged

Sync with 10.0.1 #134

merged 29 commits into from
Oct 1, 2024

Conversation

LaurenzV
Copy link
Collaborator

@LaurenzV LaurenzV commented Sep 25, 2024

Not sure what to do about harfbuzz/harfbuzz@b94a39d, since we use unicode_properties we can't just add a new adhoc category, unless we inline that crate into rustybuzz, which would be a bit unfortunate...

And also need to wait for unicode-rs/unicode-properties#10, and it would be great if you could update unicode-ccc and unicode-bidi-mirroring so I can include them, too.

@behdad
Copy link
Member

behdad commented Sep 25, 2024

Yeah the CGJ change is annoying and unfortunate. I didn't have any other bit to use. MAYBE I can change the CGJ category to Cf, then we have enough bits to store it's a CGJ in the high-byte. I'll give it a try.

@behdad
Copy link
Member

behdad commented Sep 25, 2024

Yeah the CGJ change is annoying and unfortunate. I didn't have any other bit to use. MAYBE I can change the CGJ category to Cf, then we have enough bits to store it's a CGJ in the high-byte. I'll give it a try.

Not CGJ; the variation-selectors I mean. I confused two similar changes.

@LaurenzV
Copy link
Collaborator Author

No worries, I'm sure I can figure something out! I'll try again tomrrow or so.

@LaurenzV
Copy link
Collaborator Author

I went ahead and copy-pasted it, it's not as bad as I thought. Just gotta wait for the three Unicode crates to be updated now.

@LaurenzV LaurenzV marked this pull request as ready for review September 25, 2024 21:31
behdad added a commit to harfbuzz/harfbuzz that referenced this pull request Sep 26, 2024
Instead of abusing an unused Gen_Cat value, use existing facilities
to remember variation selectors.

Addresses
harfbuzz/rustybuzz#134 (comment)
@behdad
Copy link
Member

behdad commented Sep 26, 2024

I made the change to HB to not need a new Gen_Cat, which is very slippery slope. So, please remove HB_UNICODE_GENERAL_CATEGORY_VARIATION_SELECTOR at any cost. Commit is linked above.

@RazrFalcon
Copy link
Collaborator

Updated unicode-ccc and unicode-bidi-mirroring.

@LaurenzV
Copy link
Collaborator Author

Just gotta wait for unicode_properties now.

@LaurenzV
Copy link
Collaborator Author

@RazrFalcon All ready now.

@RazrFalcon RazrFalcon merged commit 9a0ec97 into harfbuzz:master Oct 1, 2024
2 checks passed
@RazrFalcon
Copy link
Collaborator

Thanks!

@RazrFalcon
Copy link
Collaborator

@LaurenzV You have added Cargo.lock? Why?

@LaurenzV
Copy link
Collaborator Author

LaurenzV commented Oct 1, 2024

I added it because from what I know, it's best practice to check the Cargo.lock into git, right? Every crate I've seen so far does this, so I was surprised it isn't the case here.

In any case, the "trigger" for me was that running the test cases didn't work automatically, because the UNICODE_VERSION constant changes with each minor release of the crates, but in one case the minor version wasn't set to the newest in Cargo.toml, so one test didn't work by default because there was a version mismatch. Checking in the Cargo.lock solves this because then, everyone will get the same minor version.

But if it was on purpose (although I'm curious why), feel free to revert that change. :)

@LaurenzV LaurenzV deleted the 10.0.1 branch October 1, 2024 05:47
@RazrFalcon
Copy link
Collaborator

I added it because from what I know, it's best practice to check the Cargo.lock into git, right?

No idea, honestly. I personally never do this, except for resvg.

I don't mind having Cargo.lock, I was just curious about the reason behind it. All good.

@CryZe
Copy link
Contributor

CryZe commented Oct 1, 2024

It was best practice NOT to check Cargo.lock in for library crates. They changed their mind about it, in that now it's up to each repository on whether they want to check it in. The advantage is that the CI is more stable as slight breakage in dependencies can't affect unrelated PRs and it more accurately reflects the state of the repository when you check out old commits. However, you need to regularly run cargo update. If you don't do that the CI won't be able to test if the crate still even works against the latest (minor) versions of all the crates that you depend on, as would be the case if someone freshly adds your crate (or some crate that depends on it) to their project. Ideally your CI catches this as soon as possible, but it can't with an outdated checked in Cargo.lock. So this is an additional maintenance burden now (maybe some bot can regularly do it. Afaik dependabot can't). One way to go about it would be to regularly run the CI (daily or once per week) and specifically update the Cargo.lock in those automatically scheduled CI runs.

@RazrFalcon
Copy link
Collaborator

Thanks for the details!

@CryZe
Copy link
Contributor

CryZe commented Oct 1, 2024

Though one important point I noticed just now: While the crates that depend on rustybuzz will notice broken dependencies of rustybuzz when they freshly add rustybuzz to their project or run cargo update in that they see the compilation errors and can report them upstream, they usually don't run the test suite of rustybuzz themselves. So rustybuzz's CI now using fixed outdated dependencies is actually kind of bad in that, yes, the projects using rustybuzz will still notice compilation errors just fine, but they will likely not immediately notice more subtle issues (e.g. a crate that rustybuzz depends on not handling big endian correctly anymore, thus silently breaking those targets, but still compiling) and rustybuzz's CI is now with Cargo.lock checked in not recognizing such problems either until someone manually updates it. Therefore I think it's counter productive for the robustness of Rust's ecosystem for Cargo.lock files being checked into the repositories of library crates, unless they explicitly ensure CI regularly runs without a Cargo.lock / runs cargo update.

@alerque
Copy link
Member

alerque commented Oct 1, 2024

I suggest keeping the lock file tracked in Git. There are increasingly more advantages and less disadvantages to having it. It should of course be updated and tested at least prior to any releases, and of course periodically testing to make sure things work is good. On the other hand CI can be setup to ignore the lock file and test with (or with and without) freshened dependencies. This does not require perpetually bumping them if there isn't going to be a release immanent anyway and still gives a chance to have an early warning if something down or upstream is starting to surface breakage. Additionally it makes testing PRs for regressions much easier and even gives downstream consumers a known working set of dependencies if they do want to pin to them.

@RazrFalcon
Copy link
Collaborator

@CryZe I agree, but rustybuzz is a bit special here, since most dependencies do not really affect the output. ttf-parser is the only exception here, but it's "our" dependency, so we would not miss it.
So having a break due to a dependency update is nearly impossible.

@CryZe
Copy link
Contributor

CryZe commented Oct 1, 2024

@RazrFalcon That's a really good point.

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.

5 participants