-
Notifications
You must be signed in to change notification settings - Fork 566
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
Implement read-only local cache #2048
Changes from 1 commit
081c4f2
e910ff4
843448a
beac2ec
1bf9f61
609b405
8c54c2f
805e4ef
b065f20
c765ec1
6364e35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
use crate::config; | ||
use crate::errors::*; | ||
use opendal::Operator; | ||
use opendal::{layers::LoggingLayer, services::Gcs}; | ||
|
@@ -36,6 +37,15 @@ impl RWMode { | |
} | ||
} | ||
|
||
impl From<config::CacheRWMode> for RWMode { | ||
fn from(value: config::CacheRWMode) -> Self { | ||
match value { | ||
config::CacheRWMode::ReadOnly => RWMode::ReadOnly, | ||
config::CacheRWMode::ReadWrite => RWMode::ReadWrite, | ||
} | ||
} | ||
} | ||
|
||
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 are a few different types like this that have the same purpose. Should they be consolidated? 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've merged two of them together, although there's still |
||
/// A cache that stores entries in Google Cloud Storage | ||
pub struct GCSCache; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -340,42 +340,51 @@ where | |
let mut updated = false; | ||
let hit = preprocessor_cache_entry | ||
.lookup_result_digest(preprocessor_cache_mode_config, &mut updated); | ||
|
||
let mut update_failed = false; | ||
if updated { | ||
// Time macros have been found, we need to update | ||
// the preprocessor cache entry. See [`PreprocessorCacheEntry::result_matches`]. | ||
debug!( | ||
"Preprocessor cache updated because of time macros: {preprocessor_key}" | ||
); | ||
storage.put_preprocessor_cache_entry( | ||
|
||
if let Err(e) = storage.put_preprocessor_cache_entry( | ||
preprocessor_key, | ||
preprocessor_cache_entry, | ||
)?; | ||
) { | ||
debug!("Failed to update preprocessor cache: {}", e); | ||
update_failed = true; | ||
} | ||
Comment on lines
-349
to
+358
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'm unsure about this behavior. It's based on how Realistically, errors shouldn't be used here, but it seems to be how it's handled with GCS read-only, so I did it the same way. I thought about changing it, but I'd like to hear the input about someone more knowledgeable about sccache's internals first. |
||
} | ||
if let Some(key) = hit { | ||
debug!("Preprocessor cache hit: {preprocessor_key}"); | ||
// A compiler binary may be a symlink to another and | ||
// so has the same digest, but that means | ||
// the toolchain will not contain the correct path | ||
// to invoke the compiler! Add the compiler | ||
// executable path to try and prevent this | ||
let weak_toolchain_key = | ||
format!("{}-{}", executable.to_string_lossy(), executable_digest); | ||
return Ok(HashResult { | ||
key, | ||
compilation: Box::new(CCompilation { | ||
parsed_args: parsed_args.to_owned(), | ||
#[cfg(feature = "dist-client")] | ||
// TODO or is it never relevant since dist? | ||
preprocessed_input: vec![], | ||
executable: executable.to_owned(), | ||
compiler: compiler.to_owned(), | ||
cwd: cwd.to_owned(), | ||
env_vars: env_vars.to_owned(), | ||
}), | ||
weak_toolchain_key, | ||
}); | ||
} else { | ||
debug!("Preprocessor cache miss: {preprocessor_key}"); | ||
|
||
if !update_failed { | ||
if let Some(key) = hit { | ||
debug!("Preprocessor cache hit: {preprocessor_key}"); | ||
// A compiler binary may be a symlink to another and | ||
// so has the same digest, but that means | ||
// the toolchain will not contain the correct path | ||
// to invoke the compiler! Add the compiler | ||
// executable path to try and prevent this | ||
let weak_toolchain_key = | ||
format!("{}-{}", executable.to_string_lossy(), executable_digest); | ||
return Ok(HashResult { | ||
key, | ||
compilation: Box::new(CCompilation { | ||
parsed_args: parsed_args.to_owned(), | ||
#[cfg(feature = "dist-client")] | ||
// TODO or is it never relevant since dist? | ||
preprocessed_input: vec![], | ||
executable: executable.to_owned(), | ||
compiler: compiler.to_owned(), | ||
cwd: cwd.to_owned(), | ||
env_vars: env_vars.to_owned(), | ||
}), | ||
weak_toolchain_key, | ||
}); | ||
} else { | ||
debug!("Preprocessor cache miss: {preprocessor_key}"); | ||
} | ||
} | ||
} | ||
} | ||
|
@@ -491,8 +500,12 @@ where | |
.collect(); | ||
files.sort_unstable_by(|a, b| a.1.cmp(&b.1)); | ||
preprocessor_cache_entry.add_result(start_of_compilation, &key, files); | ||
storage | ||
.put_preprocessor_cache_entry(&preprocessor_key, preprocessor_cache_entry)?; | ||
|
||
if let Err(e) = storage | ||
.put_preprocessor_cache_entry(&preprocessor_key, preprocessor_cache_entry) | ||
{ | ||
debug!("Failed to update preprocessor cache: {}", e); | ||
} | ||
Comment on lines
-494
to
+508
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. Same as above. |
||
} | ||
} | ||
|
||
|
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.
could you please detail in which case someone might want to use READ_ONLY
thanks
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.
@sylvestre
Done, let me know if that's what you want. For a more specific use case, see the linked issue. I'm using sccache to speed up build times for untrusted (and short-lived) build tasks, where new items can't be added as they may not be valid, but caching commonly used libraries can still speed up build times immensely. Read-only is not being used as a security feature, as build tasks are already isolated correctly, but to reduce useless disk consumption.