google / rust_icu

rust_icu: rust bindings for ICU (International Components for Unicode) library
Apache License 2.0
113 stars 34 forks source link

feat: implement rust icu ucsdet #274

Closed Colerar closed 1 year ago

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Colerar commented 1 year ago

How to run bindgen for all current supported ICU versions? I do not find any documentation on this. And the make static-bindgen is always failed on my machine.

filmil commented 1 year ago

make static-bindgen is supposed to be the way to do that.

Any chance you can file a bug if it is not working for you?

filmil commented 1 year ago

Thank you for the PR.

It seems that some of the build infra has bitrotted, let me fix that first and I'll then get to reviewing and approving your PR.

filmil commented 1 year ago

This is blocked behind https://github.com/google/rust_icu/pull/277

filmil commented 1 year ago

https://github.com/google/rust_icu/pull/277 is now merged, could you rebase and retry please?

Colerar commented 1 year ago

Could you please approve the workflow running?

Colerar commented 1 year ago

CI failed because pregenerated bindgen/lib_*.rs is outdated, I'll try fix bindgen issue first.

filmil commented 1 year ago

Would you mind running make uprev to correct the version numbers? And please check that the resulting changes had the correct effect (uprevving only rust_icu versions).

As for bindgen, I'll go try it out locally and see that it works, and from there we're good to go.

filmil commented 1 year ago

Sadly make static-bindgen no longer works after this change:

╰─>$ make static-bindgen
mkdir -p /usr/local/google/tmp/rust_icu-fmil-target
echo top_dir: /usr/local/google/home/fmil/code/rust_icu
top_dir: /usr/local/google/home/fmil/code/rust_icu
echo pwd: /usr/local/google/home/fmil/code/rust_icu
pwd: /usr/local/google/home/fmil/code/rust_icu
docker run --tty --interactive \
                --user=89956:89939 \
                --volume=/usr/local/google/home/fmil/code/rust_icu:/src/rust_icu \
                --volume=/usr/local/google/home/fmil/.cargo:/usr/local/cargo \
                --env="RUST_ICU_MAJOR_VERSION_NUMBER=63" \
                --entrypoint="/bin/bash" \
                filipfilmar/rust_icu_testenv-63:1.73.1 \
                  "-c" "env OUTPUT_DIR=./rust_icu/rust_icu_sys/bindgen \
                  ./rust_icu/rust_icu_sys/bindgen/run_bindgen.sh"
icu-config found
mkdir: cannot create directory 'tmp': Permission denied
make: *** [Makefile:85: static-bindgen-63.stamp] Error 1

Can you try fixing pls?

In the background, I'm trying to figure out how to make this less of a chore. Sry.

filmil commented 1 year ago

On the upside, adding make static-bindgen to github workflows fails the same way static-bindgen used to fail on your end, which means we have a repro for that particular issue.

filmil commented 1 year ago

FYI: the earlier bindgen issue is now reproed and diagnosed here: https://github.com/google/rust_icu/pull/279#issuecomment-1666977742

Colerar commented 1 year ago

mkdir: cannot create directory 'tmp': Permission denied

Now the script use mktemp -d and touch (the option --suffix is relative new and not always available, and the mkdir approach may cause permission error).

filmil commented 1 year ago

Would you mind resolving the merge conflict, we can then move to merge this PR. Thanks.

filmil commented 1 year ago

This seems to be passing tests.

Could you please rebase your changes on top of current main? I would like to "rebase and merge" to keep a linear history.

And figure out whether you want to reorganize the commits or not.

filmil commented 1 year ago

Merged. Thank you for your contributions, and your patience.