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

Limit test data excluded and support HARFBUZZ_SYS_NO_PKG_CONFIG better #142

Closed
wants to merge 70 commits into from

Conversation

@spl
Copy link
Contributor

spl commented Apr 2, 2019

  • This is an alternative to #138 that builds on the work there.
  • I added better support for HARFBUZZ_SYS_NO_PKG_CONFIG being 0 or 1.
  • I expanded the Travis-CI test matrix to test the above as well as add osx.

This change is Reviewable

jdm and others added 3 commits Mar 27, 2019
* HARFBUZZ_SYS_NO_PKG_CONFIG=0 and 1
* harfbuff-sys package
* osx
@jdm
jdm approved these changes Apr 2, 2019
@spl
Copy link
Contributor Author

spl commented Apr 2, 2019

I don't understand the error.

On the PR build (HARFBUZZ_SYS_NO_PKG_CONFIG="1"):

$ cargo package --manifest-path=harfbuzz-sys/Cargo.toml
error: 1 files in the working directory contain changes that were not yet committed into git:

harfbuzz/src/hb-ot-shape-complex-use-machine.hh

to proceed despite this, pass the `--allow-dirty` flag

The command "cargo package --manifest-path=harfbuzz-sys/Cargo.toml" exited with 101.

On my build of the same:

$ cargo package --manifest-path=harfbuzz-sys/Cargo.toml
   Packaging harfbuzz-sys v0.3.1 (/home/travis/build/spl/rust-harfbuzz/harfbuzz-sys)
   Verifying harfbuzz-sys v0.3.1 (/home/travis/build/spl/rust-harfbuzz/harfbuzz-sys)
    Updating crates.io index
   Compiling cc v1.0.32
   Compiling pkg-config v0.3.14
   Compiling libc v0.2.51
   Compiling cmake v0.1.37
   Compiling servo-freetype-sys v4.0.5
   Compiling harfbuzz-sys v0.3.1 (/home/travis/build/spl/rust-harfbuzz/target/package/harfbuzz-sys-0.3.1)
   Compiling freetype v0.4.1
    Finished dev [unoptimized + debuginfo] target(s) in 58.07s
The command "cargo package --manifest-path=harfbuzz-sys/Cargo.toml" exited with 0.

Is this a Travis-CI cache issue of some sort?

@spl
Copy link
Contributor Author

spl commented Apr 2, 2019

The PR is on travis-ci.com, and mine is on travis-ci.org. Does that make a difference?

@jdm
Copy link
Member

jdm commented Apr 2, 2019

That's the same error that I saw on my PR for one of the CI builds (the other passed). I don't know what to make of it.

@spl
Copy link
Contributor Author

spl commented Apr 3, 2019

At least now something's happening on both travis-ci.com and travis-ci.org:

https://travis-ci.com/servo/rust-harfbuzz/jobs/189894765 (HARFBUZZ_SYS_NO_PKG_CONFIG="1"):

$ cargo package --manifest-path=harfbuzz-sys/Cargo.toml
   Packaging harfbuzz-sys v0.3.1 (/home/travis/build/servo/rust-harfbuzz/harfbuzz-sys)
   Verifying harfbuzz-sys v0.3.1 (/home/travis/build/servo/rust-harfbuzz/harfbuzz-sys)
    Updating crates.io index
   Compiling cc v1.0.34
   Compiling pkg-config v0.3.14
   Compiling libc v0.2.51
   Compiling cmake v0.1.37
   Compiling servo-freetype-sys v4.0.5
   Compiling harfbuzz-sys v0.3.1 (/home/travis/build/servo/rust-harfbuzz/target/package/harfbuzz-sys-0.3.1)
   Compiling freetype v0.4.1
    Finished dev [unoptimized + debuginfo] target(s) in 57.88s
error: failed to verify package tarball
Caused by:
  Source directory was modified by build.rs during cargo publish. Build scripts should not modify anything outside of OUT_DIR. Modified file:
/home/travis/build/servo/rust-harfbuzz/target/package/harfbuzz-sys-0.3.1/harfbuzz/src/hb-ot-shape-complex-use-machine.hh
To proceed despite this, pass the `--no-verify` flag.
The command "cargo package --manifest-path=harfbuzz-sys/Cargo.toml" exited with 101.

https://travis-ci.org/spl/rust-harfbuzz/jobs/515032933 (HARFBUZZ_SYS_NO_PKG_CONFIG="0"):

$ cargo package --manifest-path=harfbuzz-sys/Cargo.toml
   Packaging harfbuzz-sys v0.3.1 (/home/travis/build/spl/rust-harfbuzz/harfbuzz-sys)
   Verifying harfbuzz-sys v0.3.1 (/home/travis/build/spl/rust-harfbuzz/harfbuzz-sys)
    Updating crates.io index
   Compiling cc v1.0.34
   Compiling pkg-config v0.3.14
   Compiling libc v0.2.51
   Compiling cmake v0.1.37
   Compiling servo-freetype-sys v4.0.5
   Compiling harfbuzz-sys v0.3.1 (/home/travis/build/spl/rust-harfbuzz/target/package/harfbuzz-sys-0.3.1)
   Compiling freetype v0.4.1
    Finished dev [unoptimized + debuginfo] target(s) in 58.55s
error: failed to verify package tarball
Caused by:
  Source directory was modified by build.rs during cargo publish. Build scripts should not modify anything outside of OUT_DIR. Modified file:
/home/travis/build/spl/rust-harfbuzz/target/package/harfbuzz-sys-0.3.1/harfbuzz/util/ansi-print.cc
To proceed despite this, pass the `--no-verify` flag.
The command "cargo package --manifest-path=harfbuzz-sys/Cargo.toml" exited with 101.
spl added 3 commits Apr 3, 2019
@spl
Copy link
Contributor Author

spl commented Apr 3, 2019

Well, at least I can confirm that the last modified file time does change. Not yet sure if that's useful, but it seems to imply that cargo package is failing for a valid reason.

[2019-04-03T08:00:55Z TRACE cargo::sources::path] last modified file /home/travis/build/servo/rust-harfbuzz/target/package/harfbuzz-sys-0.3.1: 1554278454.221515537s
...
[2019-04-03T08:01:50Z TRACE cargo::sources::path] last modified file /home/travis/build/servo/rust-harfbuzz/target/package/harfbuzz-sys-0.3.1: 1554278467.852523536s

1554278467.852523536 - 1554278454.221515537 = 13.63100791 seconds

spl added 11 commits Apr 3, 2019
@spl
Copy link
Contributor Author

spl commented Apr 3, 2019

Now, when I see:

error: failed to verify package tarball
Caused by:
Source directory was modified by build.rs during cargo publish.
Build scripts should not modify anything outside of OUT_DIR.
Modified file:
.../target/package/harfbuzz-sys-0.3.1/harfbuzz/src/hb-ot-shape-complex-myanmar-machine.hh

I can confirm that its file modification time changed (and no other non-Makefile modification time changed):

-[harfbuzz-sys 0.3.1] -rw-rw-r-- 1 travis travis  12409 2019-04-03 11:36:22.227032921 +0000 hb-ot-shape-complex-myanmar-machine.hh
+[harfbuzz-sys 0.3.1] -rw-rw-r-- 1 travis travis  12409 2019-04-03 11:36:36.647224654 +0000 hb-ot-shape-complex-myanmar-machine.hh

and no content changed:

$ diff -r harfbuzz-sys/harfbuzz target/package/harfbuzz-sys-0.3.1/harfbuzz
Only in harfbuzz-sys/harfbuzz/test/fuzzing: fonts
Only in harfbuzz-sys/harfbuzz/test/shaping/data/aots: fonts
Only in harfbuzz-sys/harfbuzz/test/shaping/data/in-house: fonts
Only in harfbuzz-sys/harfbuzz/test/shaping/data/text-rendering-tests: fonts
Only in harfbuzz-sys/harfbuzz/test/subset/data: fonts

Unfortunately, this particular error and every other one appears nondeterministically.

@spl
Copy link
Contributor Author

spl commented Apr 3, 2019

Just to prove my point, triggering a rebuild in 4a6d69a caused the error to go away.

spl added 2 commits Apr 3, 2019
@spl
Copy link
Contributor Author

spl commented Apr 3, 2019

I'm at a loss. Sometimes, it's util/ansi-print.cc, as in 3b148db. Other times, it's src/hb-ot-shape-complex-myanmar-machine.h or src/hb-ot-shape-complex-use-machine.hh. Sometimes, it's HARFBUZZ_SYS_NO_PKG_CONFIG=0, other times it's HARFBUZZ_SYS_NO_PKG_CONFIG=1. And, finally, sometimes, there's no issue at all. 😱

@fasihrana
Copy link

fasihrana commented Apr 10, 2019

This just caused an issue for me publishing my own dependent crate to crates.io. I don't see an issue when building binaries, but this shows up in cargo package on fasihrana/skryn

@spl
Copy link
Contributor Author

spl commented Apr 10, 2019

This just caused an issue for me publishing my own dependent crate to crates.io. I don't see an issue when building binaries, but this shows up in cargo package on fasihrana/skryn

@fasihrana I think you're referring to #137, which was fixed in #144, and that was merged to master 2 days ago. I think @jdm is going to make a release soon.

This particular PR started as a fix for #137 but has since become an attempt to figure out yet another, more elusive issue with cargo package.

@fasihrana
Copy link

fasihrana commented Apr 10, 2019

@fasihrana I think you're referring to #137, which was fixed in #144, and that was merged to master 2 days ago. I think @jdm is going to make a release soon.

Thank you. I'll update my crate once that is released.

spl added 13 commits Apr 10, 2019
Apparently, sed is required by configure.
@jdm jdm mentioned this pull request Apr 16, 2019
@spl
Copy link
Contributor Author

spl commented Apr 16, 2019

While this was a interesting (and challenging) experiment to determine why sporadic and spurious errors were appearing when running cargo package on Travis, I can no longer reproduce these errors in the test matrix of #146. Since #144 already included the key part of this PR and #146 has a better setup, I'm closing this.

@spl spl closed this Apr 16, 2019
@spl spl deleted the spl:cargo-package branch Apr 16, 2019
@spl
Copy link
Contributor Author

spl commented Apr 16, 2019

Correction: The cargo package errors seen here are still apparent in #146. I unknowingly hid them behind after_script, a part of the job lifecycle whose errors do not affect the build status. That said, there is still nothing in this PR that warrants keeping it open.

spl added a commit to spl/rust-harfbuzz that referenced this pull request Apr 17, 2019
cargo package sporadically complains about file modifications outside
the OUT_DIR. See servo#142 for an
experiment with chasing down that issue, which can involve different
files changed at different times.

The build script is written to build out-of-tree in the OUT_DIR, but it
seems like 'configure' and/or 'make' still modifies the source tree.

This change involves creating a temporary BUILD_DIR in the OUT_DIR,
copying the source files there, building, installing into the OUT_DIR,
and removing the BUILD_DIR.
spl added a commit to spl/rust-harfbuzz that referenced this pull request Apr 17, 2019
cargo package sporadically complains about file modifications outside
the OUT_DIR. See servo#142 for an
experiment with chasing down that issue, which can involve different
files changed at different times.

The build script is written to build out-of-tree in the OUT_DIR, but
`configure` and/or `make` still modifies the source tree.

This change involves creating a temporary BUILD_DIR in the OUT_DIR,
copying the source files there, building, installing into the OUT_DIR,
and, finally, removing the BUILD_DIR.
spl added a commit to spl/rust-harfbuzz that referenced this pull request Apr 17, 2019
cargo package sporadically complains about file modifications outside
the OUT_DIR. See servo#142 for an
experiment with chasing down that issue, which can involve different
files changed at different times.

The build script is written to build out-of-tree in the OUT_DIR, but
`configure` and/or `make` still modifies the source tree.

This change involves creating a temporary BUILD_DIR in the OUT_DIR,
copying the source files there, building, installing into the OUT_DIR,
and, finally, removing the BUILD_DIR.
spl added a commit to spl/rust-harfbuzz that referenced this pull request Apr 17, 2019
`cargo package` sporadically complains about file modifications outside
`OUT_DIR`. See servo#142 for an
experiment with chasing down that issue, which can involve different
files changed at different times.

The build script is written to build out-of-tree in `OUT_DIR`, but
`configure` and/or `make` still modify the source tree.

This change involves creating a temporary `BUILD_DIR` in `OUT_DIR`,
copying the source files there, building, installing into `OUT_DIR`,
and, finally, removing `BUILD_DIR`.
bors-servo added a commit that referenced this pull request Apr 17, 2019
Avoid source changes during static build

`cargo package` sporadically complains about file modifications outside `OUT_DIR`. See #142 for an experiment with chasing down that issue, which can involve different files changed at different times.

The build script is written to build out-of-tree in `OUT_DIR`, but `configure` and/or `make` still modify the source tree.

This change involves creating a temporary `BUILD_DIR` in `OUT_DIR`, copying the source files there, building, installing into `OUT_DIR`, and, finally, removing `BUILD_DIR`.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-harfbuzz/147)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.