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

[DO-NOT-MERGE] Try out cargo-sweep #1602

Closed
wants to merge 5 commits into from

Conversation

jtgeibel
Copy link
Member

No description provided.

This assumes that cargo-sweep is already present in the cache.

I've added `|| true` so that this can bootstrap if the cache is
cleared.  This is not intended to be a long-term solution.
@sgrif
Copy link
Contributor

sgrif commented Jan 24, 2019

I'm assuming this command clears out all files that would not exist if you ran cargo clean && cargo build? If so we should open a PR to the Rust buildpack to run this as well.

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 24, 2019

That is what this command is intended to do. When it works we should advertise it heavily and encouraged people to use it lots of places.

However, this test was not terribly successful, so it would be premature to be too vocal now.

If you would like to help us get it working oh, it is always appreciated.

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 25, 2019

@jtgeibel can you give a summary of what you tested and your thoughts, if you have time?

Here is how I see it:

  • curl -LSfs https://japaric.github.io/trust/install.sh | sh -s -- --git holmgr/cargo-sweep did not work, cargo-sweep does not have release set up for it yet.
  • cargo sweep -s && .... && cargo sweep -f deleted far to much, travis does not maintain the timestamp we need. (I am working on getting cargo to maintain it more reliably.)
  • cargo sweep -i on its own should work to replace Run cargo clean on CI when the version of rustc changes #1578, but probably not worth the install from source if it is the only part working. I forgot to ask that this get tested, sorry.
  • cargo-sweep doesn't yet have an alternative for Prune some build artifacts on CI to speed up caching #1580. (I am looking into how to add it, and may have some questions.)

@jtgeibel
Copy link
Member Author

@Eh2406 to build off your summary a bit:

  • I didn't actually run the curl install command because I noticed that https://github.com/holmgr/cargo-sweep/releases does not contain the various build artifacts that the script expects. It only includes a tarball of the source. The script expects to see tarballs of the compiled executables as seen here: https://github.com/japaric/trust/releases.
  • I think that too much was deleted, but it is hard to tell exactly what was going on. The behavior seemed intermittent, as some rebuilds seemed to use cached artifacts but the next rebuild didn't. It is worth noting that our setup is a bit different in that we cargo install diesel_cli and set the CARGO_TARGET_DIR environment variable so that this builds in the directory cached by travis alongside our main cargo test run. So its possible that our particular configuration is confusing cargo-sweep, but it also appeared that dependency artifacts were being removed. My guess is that, for a dependency chain A->B->C, C is being deleted because it isn't referenced in the rebuild of A. Then, if the version of B changes then B and C both need to be built, even if the version of C has not changed.
  • Yes, I think cargo sweep -i works, but don't think it's worth adding the dependency at this time for just that feature.

@jtgeibel jtgeibel closed this Jan 31, 2019
@jtgeibel jtgeibel deleted the try-cargo-sweep branch January 31, 2019 00:02
@Eh2406
Copy link
Contributor

Eh2406 commented Jan 31, 2019

Thank you so much for the help! I am working on fixing the problems you confirmed and I will ask your help again when they all have been fixed.

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

Successfully merging this pull request may close these issues.

3 participants