Skip to content

make local worker execute command with canonicalized path,#2221

Open
zhizhi-dev wants to merge 4 commits intoTraceMachina:mainfrom
zhizhi-dev:main
Open

make local worker execute command with canonicalized path,#2221
zhizhi-dev wants to merge 4 commits intoTraceMachina:mainfrom
zhizhi-dev:main

Conversation

@zhizhi-dev
Copy link

@zhizhi-dev zhizhi-dev commented Mar 16, 2026

as rust doc current_dir say current_dir is ambiguous


This change is Reviewable

@zhizhi-dev
Copy link
Author

zhizhi-dev commented Mar 16, 2026

remote_execution.proto

  // The arguments to the command.
  //
  // The first argument specifies the command to run, which may be either an
  // absolute path, a path relative to the working directory, or an unqualified
  // path (without path separators) which will be resolved using the operating
  // system's equivalent of the PATH environment variable. Path separators
  // native to the operating system running on the worker SHOULD be used. If the
  // `environment_variables` list contains an entry for the PATH environment
  // variable, it SHOULD be respected. If not, the resolution process is
  // implementation-defined.
  //
  // Changed in v2.3. v2.2 and older require that no PATH lookups are performed,
  // and that relative paths are resolved relative to the input root. This
  // behavior can, however, not be relied upon, as most implementations already
  // followed the rules described above.
  repeated string arguments = 1;

@zhizhi-dev zhizhi-dev mentioned this pull request Mar 16, 2026
@zhizhi-dev
Copy link
Author

thread 'test_sentinel_connect_with_url_specified_master' panicked at nativelink-store/tests/redis_store_test.rs:848:42:
Working spec: Error { code: DeadlineExceeded, messages: ["deadline has elapsed", "Timeout while connecting to redis with url: redis+sentinel://127.0.0.1:46093/?sentinelServiceName=specific_master"] }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Why ?

@zhizhi-dev
Copy link
Author

Hi, @MarcusSorealheis
please help me te review this code

@MarcusSorealheis
Copy link
Collaborator

@zhizhi-dev I'm reviewing now but in the short term, the tests fail, so we need to get over that issue.

@palfrey
Copy link
Member

palfrey commented Mar 17, 2026

@zhizhi-dev I'm reviewing now but in the short term, the tests fail, so we need to get over that issue.

Test failure was

thread 'test_sentinel_connect_with_url_specified_master' panicked at nativelink-store/tests/redis_store_test.rs:848:42:
Working spec: Error { code: DeadlineExceeded, messages: ["deadline has elapsed", "Timeout while connecting to redis with url: redis+sentinel://127.0.0.1:46093/?sentinelServiceName=specific_master"] }

which is weird and I'm not sure why that one occurs as the logs before it seem fine, and it feels unrelated to this issue so re-running.

PathBuf::from(&args[0])
};

let mut command_builder = process::Command::new(program);
Copy link
Member

Choose a reason for hiding this comment

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

@zhizhi-dev Can you add a test for this please? Possibly split out the canonicalisation as a new function and test that?

let program = if Path::new(&args[0]).components().count() > 1 {
PathBuf::from(&self.work_directory)
.join(&command_proto.working_directory)
.join(&args[0])
Copy link
Member

Choose a reason for hiding this comment

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

BTW there's a warning flag on this one

warning: the borrowed expression implements the required traits
   --> nativelink-worker/src/running_actions_manager.rs:937:23
    |
937 |                 .join(&args[0])
    |                       ^^^^^^^^ help: change this to: `args[0]`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be a false positive?

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.

3 participants