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

build script always re-executed #24

Closed
tforgione opened this issue May 16, 2019 · 9 comments
Closed

build script always re-executed #24

tforgione opened this issue May 16, 2019 · 9 comments

Comments

@tforgione
Copy link

tforgione commented May 16, 2019

Hi!

First of all, thank you for this great library!

I open this issue because it seems that the build script is always retriggered (see example).

I guess it's due to those lines. I'm not super sure about it, but it seems that those files aren't changed, they might just be overwritten, making cargo think that they are changed, are re-trigger the build.

Is this an expected behaviour ?

Example

Here is what I get if a create a simple hello world, add hyphenation to the dependencies and compile it twice in a row.

╭── thomas::~/hyphentest ‹master*›
╰▶  cargo build # First build
    Updating crates.io index
   Compiling proc-macro2 v0.4.30
   Compiling unicode-xid v0.1.0
   Compiling syn v0.15.34
   Compiling num-traits v0.2.6
   Compiling serde v1.0.91
   Compiling byteorder v1.3.1
   Compiling autocfg v0.1.2
   Compiling fnv v1.0.6
   Compiling bincode v1.1.4
   Compiling quote v0.6.12
   Compiling serde_derive v1.0.91
   Compiling atlatl v0.1.2
   Compiling hyphenation_commons v0.7.1
   Compiling hyphenation v0.7.1
   Compiling hyphentest v0.1.0 (/home/thomas/hyphentest)
    Finished dev [unoptimized + debuginfo] target(s) in 48.89s
╭── thomas::~/hyphentest ‹master*›
╰▶  cargo build # Second build
   Compiling hyphenation v0.7.1
   Compiling hyphentest v0.1.0 (/home/thomas/hyphentest)
    Finished dev [unoptimized + debuginfo] target(s) in 0.58s

As you can see, on the second compilation, hyphenation is re-compiled.

@baskerville
Copy link
Contributor

The problem is that build.rs prints the following line: cargo:rerun-if-changed=hyphenation_commons/src/dictionary.rs. If you download the sources for the 0.7.1 crate (with cargo-clone for example), you'll notice that there's no hyphenation_commons directory in the root. And, unfortunately, it seems that cargo thinks that a missing path always changes.

@baskerville
Copy link
Contributor

I discovered another quirk: the 0.7.1 crate ships a target directory (that weighs 83 MiB)!

@baskerville
Copy link
Contributor

I've fixed #24, #18, #19, #20 and #16 in my own fork.

@MalteT
Copy link

MalteT commented Jan 18, 2020

@baskerville Could you perhaps open a PR? #24 is quite annoying and textwrap uses this crate instead of your fork 🤔

@tapeinosyne
Copy link
Owner

tapeinosyne commented Jan 22, 2020

Apologies for being unresponsive.

Yes, unintended recompilation is one of two egregious build issues that affect the current version of hyphenation (the other being build time). I'll look over @baskerville's fixes and incorporate them in the next release, which should happen in the upcoming weeks.

@MalteT Build issues aside, hyphenation in its present state is not very suitable for something like textwrap: the bundled dictionaries are bloated in size and cannot be embedded individually, which is arguably a poor fit for CLI applications. The dictionaries will switch to a more compact representation in 0.8 (if possible) or 1.0 (at the latest). I'll submit a PR myself when this happens.

@baskerville
I discovered another quirk: the 0.7.1 crate ships a target directory (that weighs 83 MiB)!

I'm not sure I follow: target is not part of the package; it's where cargo writes build artifacts, which include some hefty dependencies (notably serde at some forty-odd MB).

@baskerville
Copy link
Contributor

I discovered another quirk: the 0.7.1 crate ships a target directory (that weighs 83 MiB)!

I'm not sure I follow: target is not part of the package; it's where cargo writes build artifacts, which include some hefty dependencies (notably serde at some forty-odd MB).

I don't remember how I came to that conclusion, sorry.

@MathieuDuponchelle
Copy link

Hey @tapeinosyne , gentle ping to ask if you can bring in @baskerville's patches that address unintended recompilation :)

@tapeinosyne
Copy link
Owner

As of 8c88e89, hyphenation should no longer rebuild unless requested (via build_dictionaries) or required (for changes in the normalization form).

(It took a while. Thank you for your patience.)

@MathieuDuponchelle
Copy link

You rock, thanks :)

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

No branches or pull requests

5 participants