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

Move touch command into makefile.cargo #58

Merged
merged 1 commit into from Feb 5, 2016
Merged

Conversation

@mbrubeck
Copy link
Contributor

mbrubeck commented Jan 5, 2016

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jan 5, 2016

I worried in #54 that this change might cause spurious re-compiles, but in my testing it does not.

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jan 5, 2016

…and the Travis build failed with aclocal-1.14: command not found. Great.

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jan 5, 2016

I forgot to escape the braces in the makefile. When I fix it, the build succeeds but it does cause spurious rebuilds. So I'll need to make it smarter.

@mbrubeck mbrubeck force-pushed the mbrubeck:touch branch from 25c61ab to f990817 Jan 5, 2016
@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jan 5, 2016

Updated to fix the Makefile command and to ensure that the touch command runs only once. This will still cause one extra re-build in many cases, but the extra build is very fast because it doesn't actually re-compile Harfbuzz. (So the first cargo build causes a full compile; the second one causes a fast no-op build; and the third one correctly does nothing at all.)

I can't think of any way of totally eliminating the spurious re-build.

@metajack
Copy link
Contributor

metajack commented Jan 5, 2016

Have you tried just removing the generated files and running autogen as the first step?

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jan 6, 2016

Have you tried just removing the generated files and running autogen as the first step?

That should work, though Harfbuzz’s autogen.sh depends on both autoconf and Ragel, which would add two new prerequisites to the Servo build. Installing Ragel would be an additional (but small) hassle for Windows users.

@mbrubeck mbrubeck force-pushed the mbrubeck:touch branch from f990817 to f5f929d Jan 6, 2016
Fixes #54, fixes servo/servo#8890.
@mbrubeck mbrubeck force-pushed the mbrubeck:touch branch from f5f929d to 331eddc Jan 6, 2016
@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jan 6, 2016

Have you tried just removing the generated files and running autogen as the first step?

I implemented this, and it has the same problems as the current PR. autogen.sh touches several dozen files in the source directory, and the timestamps of these generated files cause Cargo to re-run the build script.

However, I found that adding an explicit exclude list to Cargo.toml fixes the problem of extra rebuilds. I've updated my original PR to add this. r? @metajack

(We could additionally remove all the generated files from the tree and run autogen.sh, but this requires a much larger exclude list and also adds new build dependencies, as mentioned above.)

@manfredbrandl
Copy link

manfredbrandl commented Jan 21, 2016

@metajack is there still a problem with this PR?

@metajack
Copy link
Contributor

metajack commented Feb 5, 2016

@bors-servo r+

Thanks for dealing with this. autotools stuff is never fun.


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Feb 5, 2016

@bors-servo r=metajack

@bors-servo
Copy link
Contributor

bors-servo commented Feb 5, 2016

📌 Commit 331eddc has been approved by metajack

@bors-servo
Copy link
Contributor

bors-servo commented Feb 5, 2016

Testing commit 331eddc with merge d831bfe...

bors-servo added a commit that referenced this pull request Feb 5, 2016
Move touch command into makefile.cargo

Fixes #54, fixes servo/servo#8890.

r? @larsbergstrom or @metajack

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/rust-harfbuzz/58)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 5, 2016

☀️ Test successful - travis

@bors-servo bors-servo merged commit 331eddc into servo:master Feb 5, 2016
3 checks passed
3 checks passed
code-review/reviewable Review complete: all files reviewed, all discussions resolved
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
mbrubeck added a commit to mbrubeck/rust-harfbuzz that referenced this pull request Feb 5, 2016
This reverts servo#58.  The patch worked fine when building from git but broke when
published to crates.io, because `cargo publish` does not package files in the
`exclude` list.
bors-servo added a commit that referenced this pull request Feb 8, 2016
Revert "Move touch command into makefile.cargo"

This reverts #58.  The patch worked fine when building from git but broke when published to crates.io, because `cargo publish` does not package files in the `exclude` list. :(

r? @metajack

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/rust-harfbuzz/60)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.