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

Deprecate implicit setting of --remote-cache-{read,write,eager-fetch} with --remote-execution #15900

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 34 additions & 10 deletions src/python/pants/option/global_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
is_in_container,
pants_version,
)
from pants.base.deprecated import deprecated_conditional
from pants.base.glob_match_error_behavior import GlobMatchErrorBehavior
from pants.engine.environment import CompleteEnvironment
from pants.engine.internals.native_engine import PyExecutor
Expand Down Expand Up @@ -1747,17 +1748,40 @@ def validate_instance(cls, opts):
)
)

if opts.remote_execution and (opts.remote_cache_read or opts.remote_cache_write):
raise OptionsError(
softwrap(
"""
`--remote-execution` cannot be set at the same time as either
`--remote-cache-read` or `--remote-cache-write`.
# TODO: When this deprecation triggers, the relevant TODO(s) in `context.rs` should be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should all of these TODOs be tagged with the same issue number to make finding them easier in the future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm, whoops: this was set to auto-merge. Yes, possibly. But blame should point to this PR fairly easily (which was why I made it a bi-directional reference).

# removed as well.
deprecated_conditional(
lambda: opts.remote_execution and opts.remote_cache_eager_fetch,
removal_version="2.15.0.dev0",
entity="Setting `--remote-execution` at the same time as `--remote-cache-eager-fetch`.",
hint=softwrap(
"""
Use of `--remote-execution` currently implies use of
`--remote-cache-eager-fetch=false`, but in future versions, use of eager fetch with
remote execution will be optional. To preserve the current behavior in future
versions, `--remote-cache-eager-fetch` should be disabled.
"""
),
)

If remote execution is enabled, it will already use remote caching.
"""
)
)
# TODO: When this deprecation triggers, the relevant TODO(s) in `context.rs` should be
# removed as well.
deprecated_conditional(
lambda: (
opts.remote_execution
and (not opts.remote_cache_read or not opts.remote_cache_write)
),
removal_version="2.15.0.dev0",
entity="Using `--remote-execution` without setting `--remote-cache-read` and `--remote-cache-write`.",
hint=softwrap(
"""
Use of `--remote-execution` currently implies use of a remote cache, but in future
versions, use of the remote cache with remote execution will be optional. To
preserve the current behavior in future versions, both `--remote-cache-read` and
`--remote-cache-write` should be enabled.
"""
),
)

if opts.remote_execution and not opts.remote_execution_address:
raise OptionsError(
Expand Down
4 changes: 2 additions & 2 deletions src/rust/engine/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,10 +340,10 @@ impl Core {
)?;

// TODO: Until we can deprecate letting the flag default, we implicitly disable
// eager_fetch when remote execution is in use.
// eager_fetch when remote execution is in use. See the TODO in `global_options.py`.
let eager_fetch = remoting_opts.cache_eager_fetch && !remoting_opts.execution_enable;
// TODO: Until we can deprecate letting remote-cache-{read,write} default, we implicitly
// enable them when remote execution is in use.
// enable them when remote execution is in use. See the TODO in `global_options.py`.
let remote_cache_read = exec_strategy_opts.remote_cache_read || remoting_opts.execution_enable;
let remote_cache_write =
exec_strategy_opts.remote_cache_write || remoting_opts.execution_enable;
Expand Down