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

Building now requires installing cmake #3175

Open
echeran opened this issue Mar 6, 2023 · 6 comments
Open

Building now requires installing cmake #3175

echeran opened this issue Mar 6, 2023 · 6 comments
Assignees
Labels
C-process Component: Team processes S-small Size: One afternoon (small bug fix or enhancement) T-task Type: Tracking thread for a non-code task

Comments

@echeran
Copy link
Contributor

echeran commented Mar 6, 2023

After merging the latest from upstream main, running from the root the command cargo check --tests --all-features gives the following error message, indicating that the freetype-sys v0.13.1 dependency requires a native installation of cmake in order to install.

If cmake is required to build, then it should be mentioned in CONTRIBUTING.md.

It probably wouldn't have been picked up by Github CI if the VM runners have cmake preinstalled. They have many compilers and build tools preinstalled.

Abridged console output:

...
   Compiling camino v1.1.3
    Checking semver-parser v0.10.2
    Checking ciborium-ll v0.2.0
error: failed to run custom build command for `freetype-sys v0.13.1`

Caused by:
  process didn't exit successfully: `/usr/local/google/home/elango/oss/icu4x/target/debug/build/freetype-sys-51a9ae8ad2cf60cc/build-script-build` (exit status: 101)
  --- stdout
...
 running: "cmake" "/usr/local/google/home/elango/.cargo/registry/src/github.com-1ecc6299db9ec823/freetype-sys-0.13.1/freetype2" "-DWITH_BZip2=OFF" "-DWITH_HarfBuzz=OFF" "-DWITH_PNG=OFF" "-DWITH_ZLIB=OFF" "-DCMAKE_INSTALL_PREFIX=/usr/local/google/home/elango/oss/icu4x/target/debug/build/freetype-sys-aea398d121abe122/out" "-DCMAKE_C_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_C_COMPILER=/usr/bin/cc" "-DCMAKE_CXX_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_CXX_COMPILER=/usr/bin/c++" "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_ASM_COMPILER=/usr/bin/cc" "-DCMAKE_BUILD_TYPE=Release"

  --- stderr
  thread 'main' panicked at '
  failed to execute command: No such file or directory (os error 2)
  is `cmake` not installed?

  build script failed, must exit now', /usr/local/google/home/elango/.cargo/registry/src/github.com-1ecc6299db9ec823/cmake-0.1.49/src/lib.rs:1104:5
...
@sffc
Copy link
Member

sffc commented Mar 6, 2023

Since when have we had cmake in our dep tree? We should avoid native deps and generally do.

@echeran
Copy link
Contributor Author

echeran commented Mar 6, 2023

It's from icu_harfbuzz. Running cargo tree indicates the following dependency graph path: icu_harfbuzz v0.7.0 -> harfbuzz-sys v0.5.0 -> freetype-sys v0.13.1.

fyi @hsivonen

@sffc
Copy link
Member

sffc commented Mar 6, 2023

harfbuzz-sys appears to have features to disable the native compilation; can we do that?

https://github.com/servo/rust-harfbuzz/blob/master/harfbuzz-sys/Cargo.toml

@echeran echeran added T-task Type: Tracking thread for a non-code task C-process Component: Team processes S-small Size: One afternoon (small bug fix or enhancement) labels Mar 7, 2023
@sffc
Copy link
Member

sffc commented Mar 30, 2023

Discussion: If there's no way to avoid the harfbuzz-sys dependency, we should remove icu_harfbuzz from the Cargo workspace and run it on CI as part of its own job, with the following notes:

  1. Document how to run the harfbuzz tests locally
  2. Finish the harfbuzz integration work first to see if we can avoid the harfbuzz-sys dep

@sffc sffc self-assigned this Mar 30, 2023
@sffc sffc added this to the 1.x Priority ⟨P2⟩ milestone Mar 30, 2023
@hsivonen
Copy link
Member

harfbuzz-sys appears to have features to disable the native compilation; can we do that?

https://github.com/servo/rust-harfbuzz/blob/master/harfbuzz-sys/Cargo.toml

Not trivially. We'd need to change something else related to testing. Already tried flipping the cargo feature:
#2839 (comment)

@sffc
Copy link
Member

sffc commented Apr 1, 2023

If servo/rust-harfbuzz#197 lands, then we should be able to implement the bindings without requiring harfbuzz-sys, which has the effect of mostly solving this issue (although we may still want a way to run HarfBuzz tests in CI).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-process Component: Team processes S-small Size: One afternoon (small bug fix or enhancement) T-task Type: Tracking thread for a non-code task
Projects
None yet
Development

No branches or pull requests

3 participants