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

Allow using preprocessor mode from env without a disk config #1942

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

Alphare
Copy link
Contributor

@Alphare Alphare commented Nov 6, 2023

If we set SCCACHE_DIRECT=true but not disk_dir or disk_sz, we end up with None which sets use_preprocessor_cache_mode to false.

But if we run:

./target/debug/sccache --start-server
./target/debug/sccache --show-stats

We can see that it is uses the local disk by default.

After this change, this works:
SCCACHE_DIRECT=true ./target/debug/sccache --start-server ./target/debug/sccache --show-stats

If we set `SCCACHE_DIRECT=true` but not `disk_dir` or `disk_sz`, we
end up with `None` which sets `use_preprocessor_cache_mode` to `false`.

But if we run:
```
./target/debug/sccache --start-server
./target/debug/sccache --show-stats
```

We can see that it is uses the local disk by default.

After this change, this works:
`SCCACHE_DIRECT=true ./target/debug/sccache  --start-server
./target/debug/sccache --show-stats`
@Alphare
Copy link
Contributor Author

Alphare commented Nov 6, 2023

@sylvestre this should fix #1941 but highlighted a deeper issue with the config system.

IIUC the config from env is merged with the config file's at a coarse granularity (like azure, disk, etc.).
The user can then get into a situation where they've defined a disk_dir and disk_sz in the config file, but only set use_preprocessor_cache_mode with the environment variable: this means that the two first settings will be ignored by sccache.

Is this in scope for a fix? I think it might be a bit involved since we'd need to define the merge logic either manually for every field (which is brittle, not future-proof and prone to errors), or use some form of reflection to loop through the fields.

@sylvestre
Copy link
Collaborator

looks like you didn't rebase :)

@sylvestre
Copy link
Collaborator

if you have time, yeah, it would be nice to add some extra checks

@sylvestre sylvestre merged commit 8f4c221 into mozilla:main Nov 6, 2023
39 checks passed
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.

None yet

2 participants