-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(build-dir): Reorganize build-dir layout #15947
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So while this reduces the "max content per directory" (since Should we change from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could we expand a bit more on the benefit of the proposed change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There can be performance issues on Windows when a directory has a lot of content. We do this prefix-directory stuff for the index and for the build-dir workspace hash. This would be extending it to the build units within the package dir. As Ross brought up, we don't have guidance on how big is big, what the growth will look like, etc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we share this layout with the shared cache, then it will likely be more important. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I spent some time getting up to speed on Windows path issues. (goodness, its a deep rabbit hole) As far as I can tell the main issue is Windows has much more restrictive path length than linux. (see #9770) That said I also found an article that claims a flat structure is better on ext4 for linux. Given the long paths issue on windows, perhaps it would be better to shorten names to help mitigate this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I can tell, these are separate conversations, so I moved path lengths over to https://github.com/rust-lang/cargo/pull/15947/files#r2402207606 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. As for the original suggestion
I lean towards not splitting these up. It keeps things simple and its questionable if there is even a performance issue in the first place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this is unspecified, we at least ahve the freedom to change it over time. I think we should at minimum do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that is a good idea 👍 The code for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made this change in the latest push. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need a test for
cargo clean -p foo
. Haven't looked at how thats implemented but might at least be a reason forname/hash
rather thanname-hash