DanielKeep / cargo-script

Cargo script subcommand
Other
729 stars 37 forks source link

Cache expressions in a fixed directory (was: "Cache compilations of dependencies") #16

Closed nixpulvis closed 7 years ago

nixpulvis commented 8 years ago

Running cargo script on something with dependencies is very painful due to the repeated builds.

DanielKeep commented 8 years ago

Insofar as I'm aware, there's nothing I can do about this. Cargo itself would need to do caching of compiled dependencies. cargo-script already tries to cache the final, compiled executable (and the intermediate products specific to that one executable), but that's as far as it can go.

As such, I'm closing this.

sorear commented 8 years ago

If you use --pkg-path , then cargo's built-in caching will work. Perhaps this could be added as a hint to the README?

nixpulvis commented 8 years ago

@sorear Or maybe a default?

DanielKeep commented 8 years ago

Ok, we might be talking across purposes. All that --pkg-path does is it changes the directory where cargo-script writes the package. Nothing else is different. The directory is automatically generated based on the full path of the script, and the options you passed to cargo-script. In other words, using --pkg-path on a script that's being invoked from the same place with the same options shouldn't involve any less compilation effort than not using --pkg-path.

sorear commented 8 years ago

@DanielKeep maybe shouldn't, but it does.

shell$ cargo script -d serde_json -e '2+3'
    Updating registry `https://github.com/rust-lang/crates.io-index`
   Compiling serde v0.7.0
   Compiling num v0.1.31
   Compiling serde_json v0.7.0
   Compiling expr v0.1.0 (file:///Users/sorear/.multirust/toolchains/nightly/cargo/.cargo/script-cache/expr-c3096e21eb1704b2)
5
shell$ cargo script -d serde_json -e '2+4'
    Updating registry `https://github.com/rust-lang/crates.io-index`
   Compiling serde v0.7.0
   Compiling num v0.1.31
   Compiling serde_json v0.7.0
   Compiling expr v0.1.0 (file:///Users/sorear/.multirust/toolchains/nightly/cargo/.cargo/script-cache/expr-63c9b009bed1ddd4)
6
shell$ cargo script --pkg-path=/tmp/asdf5 -d serde_json -e '2+5'
    Updating registry `https://github.com/rust-lang/crates.io-index`
   Compiling num v0.1.31
   Compiling serde v0.7.0
   Compiling serde_json v0.7.0
   Compiling expr v0.1.0 (file:///tmp/asdf5)
7
shell$ cargo script --pkg-path=/tmp/asdf5 -d serde_json -e '2+6'
   Compiling expr v0.1.0 (file:///tmp/asdf5)
8
DanielKeep commented 8 years ago

@sorear That's a completely unrelated thing; if you were talking specifically about expressions, you should have said so earlier!

The cache directory for expressions is derived from a hash of the contents, because there's no name to use. As a result, every expression gets a different cache entry. This is so that re-running expressions doesn't require a recompile.

I could make cargo-script dump all expressions to the same directory, but then it would have to recompile after every change to the expression. Another possible issue is that it wouldn't interact with the cache cleanup code at all; your target directory would just get bigger over time as it accumulates compiled dependencies.

That said, expressions tend to be very, very small, so the recompilation may not be a huge issue.

On a personal note, I'm also a little annoyed at having to address this, when it's really Cargo's problem to solve. :P

I'll have a think about it.

DanielKeep commented 8 years ago

After playing with this for a while, I managed to find something that appears to work. Specifically, unless you're using --pkg-path, cargo-script will now use CARGO_TARGET_DIR to get Cargo to write all compilation artifacts to a single, shared directory.

Now, I've observed a few effects with this change:

Specifically, it used to get confused if you did cargo script -e 0 followed by cargo script -e 1; the second command would do no compilation and output 0.

It's pretty clear that Cargo is not supposed to be used like this. I've added more checks and metadata to try and avoid situations where Cargo might get confused, but I'm still not 100% confident. I've merged the changes into master because it makes using cargo-script much smoother, but I'd like to get it into some people's hands before making a proper release.

As such, I'd appreciate it if you could install the current master and use it for a little while and see if anything wonky happens. If you want to override the use of the binary cache for testing, you can use the --use-shared-binary-cache=[yes|no] option. You should be able to directly install the commit in question like so:

cargo uninstall cargo-script
cargo install --git https://github.com/DanielKeep/cargo-script.git --rev daad2c8

... except this was apparently broken until, like, 14 hours ago, so unless you have a super recent version of Cargo, you'll need to do this:

git clone https://github.com/DanielKeep/cargo-script.git --single-branch --branch set-cargo-target-dir --depth 1
cargo install --path cargo-script

I'll bump the thread in a week if I don't get any negative feedback before then.

CC @durka who I suspect uses cargo-script more than I do. :P

DanielKeep commented 8 years ago

Bump! So, has anyone noticed any problems or issues with this branch?

sorear commented 8 years ago

I haven't had occasion to use it since your change, sorry.

durka commented 8 years ago

Nothing has exploded here yet. But I want to try it with multirust-rs. On Mar 20, 2016 04:17, "sorear" notifications@github.com wrote:

I haven't had occasion to use it since your change, sorry.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/DanielKeep/cargo-script/issues/16#issuecomment-198870543

DanielKeep commented 7 years ago

Closing this because this has been, as far as I'm aware, fixed for bloody ages.