Skip to content

/ servo

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

Mach: Add `mach clean-cargo-cache` command #16593

Merged
merged 2 commits into from May 8, 2017
Merged

Mach: Add `mach clean-cargo-cache` command #16593

merged 2 commits into from May 8, 2017

Conversation

@UK992
Copy link
Contributor

UK992 commented Apr 24, 2017 •
edited

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@highfive
Copy link

highfive commented Apr 24, 2017

Heads up! This PR modifies the following files:
@UK992 UK992 force-pushed the UK992:clean-cargo-cache branch from da57564 to 2c3b465 Apr 28, 2017
@UK992
Copy link
Contributor Author

UK992 commented Apr 28, 2017

r? @wafflespeanut
@highfive highfive assigned wafflespeanut and unassigned nox Apr 28, 2017
@wafflespeanut
wafflespeanut reviewed Apr 30, 2017
View changes
Copy link
Member

wafflespeanut left a comment

Just had a brief look. Will take a closer look tomorrow.
python/servo/bootstrap_commands.py
if os.path.isfile(path):
return os.path.getsize(path) / (1024 * 1024.0)
total_size = 0
for dirpath, dirnames, filenames in os.walk(path):

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut Apr 30, 2017

Member

I'm not really comfortable doing a os.walk through the cache, without knowing how long it takes for, say, 50 outdated crates? (which, I guess, is a perfectly normal situation for people who build servo very often).

While displaying crate sizes is a nice feedback, walking through checked out repositories (and crates) is probably a bad idea (what if the repository is huge? I'm sure python will suck on that!). So, we should see how long it takes if we don't walk (i.e., don't display size information). Then, depending on the difference, we should either remove that code, or have a flag for disabling that (since we don't want to see the size information all the time).

Also, the back-ported scandir module is worth looking at for walking and getting file info.

This comment has been minimized.

Sign in to view
@UK992

UK992 Apr 30, 2017 •
edited

Author Contributor

scandir on Windows needs Visual C++ Compiler for Python 2.7

update:
In my case (to delete 3776.33 MB):
with os.walk: 27s
with scandir: 16s

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut May 2, 2017

Member

That's not much of a difference. I don't think having that dependency is worth it. Let's stick to os.walk then. Also, what if we avoid walking entirely? (i.e., don't display sizes?)

This comment has been minimized.

Sign in to view
@UK992

UK992 May 8, 2017 •
edited

Author Contributor

6 seconds

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut May 8, 2017 •
edited

Member

Yeah, that's expected. So, it's good to have it as an option. Anyway, feel free to r=me once you've amended the fixup :)
python/servo/bootstrap_commands.py
if exist_item not in current_crate:
crate_count += 1
removing_anything = True
if int(crate_count) >= int(keep) or not current_crate:

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut Apr 30, 2017

Member

I'm curious how tracking the keep count helps in keeping the "N" most recent deps. It may probably work for downloaded crates, but the git repos have their commit hashes instead of versions, which means we're not really doing a great job there, right?

This comment has been minimized.

Sign in to view
@UK992

UK992 Apr 30, 2017 •
edited

Author Contributor

for git packages i use the same logic as in find_dep_path_newest, based on modification date.
@wafflespeanut
Copy link
Member

wafflespeanut commented Apr 30, 2017

Oh, and I have a nice (but ugly) hack in mind, which is, to make use of os.stat to get directory info (which has the mtime and ctime) which will help us to track the crates based on their creation/modification times (which may work for both git repos and downloaded crates).

(also pinging @aneeshusa for comments)
@wafflespeanut
Copy link
Member

wafflespeanut commented May 2, 2017

@UK992 Is this still WIP? Do you have something else in mind? If not, I'll go over it once more and we'll land it.
@UK992 UK992 changed the title (WIP) Mach: Add `mach clean-cargo-cache` command Mach: Add `mach clean-cargo-cache` command May 2, 2017
@UK992
Copy link
Contributor Author

UK992 commented May 2, 2017

It's ready for review.
@wafflespeanut
Copy link
Member

wafflespeanut commented May 8, 2017

Looks good to me. Can you squash the fixup appropriately?

@bors-servo delegate+
@bors-servo
Copy link
Contributor

bors-servo commented May 8, 2017

✌️ @UK992 can now approve this pull request
UK992 added 2 commits Apr 24, 2017
@UK992
Add `mach clean-cargo-cache` command
66ea4b2
@UK992
Update .travis.yml
Loading status checks…
63b19c5
@UK992 UK992 force-pushed the UK992:clean-cargo-cache branch from 85b7fa9 to 63b19c5 May 8, 2017
@UK992
Copy link
Contributor Author

UK992 commented May 8, 2017

@bors-servo r=wafflespeanut
@bors-servo
Copy link
Contributor

bors-servo commented May 8, 2017

📌 Commit 63b19c5 has been approved by wafflespeanut
@bors-servo
Copy link
Contributor

bors-servo commented May 8, 2017

⌛ Testing commit 63b19c5 with merge f6bd158...
bors-servo added a commit that referenced this pull request May 8, 2017
@bors-servo
Auto merge of #16593 - UK992:clean-cargo-cache, r=wafflespeanut
Loading status checks…
f6bd158 
Mach: Add `mach clean-cargo-cache` command

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16593)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 8, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: wafflespeanut
Pushing f6bd158 to master...
@bors-servo bors-servo merged commit 63b19c5 into servo:master May 8, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@UK992 UK992 mentioned this pull request May 8, 2017
Merged
bors-servo added a commit that referenced this pull request May 9, 2017
@bors-servo
Auto merge of #16767 - UK992:travis, r=jdm
Loading status checks…
8ff484d 
Try to re-enable unit tests on travis-ci

After #16573 and #16593, issue #15076 _could_ be fixed.

Edit:
Closes #14723
Closes #15076

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16767)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 9, 2017
@UK992
servo: Merge #16767 - Try to re-enable unit tests on travis-ci (from …
ddb826c 
…UK992:travis); r=jdm

After servo/servo#16573 and servo/servo#16593, issue servo/servo#15076 _could_ be fixed.

Edit:
Closes #14723
Closes #15076

Source-Repo: https://github.com/servo/servo
Source-Revision: 8ff484d3fd0b092d64af8b0a2cfa51b318631b82

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : b2ee28ecf18792f3a1ebf50bcb548ede41fb2e70
aethanyc pushed a commit to aethanyc/gecko-dev that referenced this pull request May 10, 2017
@UK992
servo: Merge #16767 - Try to re-enable unit tests on travis-ci (from …
a82063b 
…UK992:travis); r=jdm

After servo/servo#16573 and servo/servo#16593, issue servo/servo#15076 _could_ be fixed.

Edit:
Closes #14723
Closes #15076

Source-Repo: https://github.com/servo/servo
Source-Revision: 8ff484d3fd0b092d64af8b0a2cfa51b318631b82
@aneeshusa aneeshusa mentioned this pull request Jul 6, 2017
Closed
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
@marco-c
servo: Merge #16767 - Try to re-enable unit tests on travis-ci (from …
95de743 
…UK992:travis); r=jdm

After servo/servo#16573 and servo/servo#16593, issue servo/servo#15076 _could_ be fixed.

Edit:
Closes #14723
Closes #15076

Source-Repo: https://github.com/servo/servo
Source-Revision: 8ff484d3fd0b092d64af8b0a2cfa51b318631b82

UltraBlame original commit: 2445c1a53fe2bf66317fcbb495a528eb68fdabc3
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
@marco-c
servo: Merge #16767 - Try to re-enable unit tests on travis-ci (from …
542c3fe 
…UK992:travis); r=jdm

After servo/servo#16573 and servo/servo#16593, issue servo/servo#15076 _could_ be fixed.

Edit:
Closes #14723
Closes #15076

Source-Repo: https://github.com/servo/servo
Source-Revision: 8ff484d3fd0b092d64af8b0a2cfa51b318631b82

UltraBlame original commit: 2445c1a53fe2bf66317fcbb495a528eb68fdabc3
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
@marco-c
servo: Merge #16767 - Try to re-enable unit tests on travis-ci (from …
bfcdb2e 
…UK992:travis); r=jdm

After servo/servo#16573 and servo/servo#16593, issue servo/servo#15076 _could_ be fixed.

Edit:
Closes #14723
Closes #15076

Source-Repo: https://github.com/servo/servo
Source-Revision: 8ff484d3fd0b092d64af8b0a2cfa51b318631b82

UltraBlame original commit: 2445c1a53fe2bf66317fcbb495a528eb68fdabc3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wafflespeanut wafflespeanut

Assignees

@wafflespeanut wafflespeanut

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@UK992 @highfive @wafflespeanut @bors-servo @nox
You can’t perform that action at this time.