Make rbe_configs_gen more windows friendly. #954
Conversation
rubensf
requested review from
DaveGay,
eytankidron,
rbe-toolchains-pr-bot and
smukherj1
as code owners
pkg/rbeconfigsgen/rbeconfigsgen.go
Outdated
| return "", fmt.Errorf("failed to harden symlink %q in %q pointing to %q: %w", s, cppConfigDir, resolvedPath, err) | ||
| if err != nil { | ||
| log.Printf("WARNING: unable to list symlinks in the C++ config generation build output directory: %v", err) | ||
| } else { |
There was a problem hiding this comment.
Don't think this else is necessary. Set out to blank string if the command to list symlinks fails and you'll have an empty slice in symlinks and the for loop will never be entered. Avoids nesting the control flow
| } | ||
| if _, err := d.execCmd("ln", "-f", resolvedPath, s); err != nil { | ||
| return "", fmt.Errorf("failed to harden symlink %q in %q pointing to %q: %w", s, cppConfigDir, resolvedPath, err) | ||
| if err != nil { |
There was a problem hiding this comment.
Can you check if find returns an error if symlinks aren't found? If the command behaves similar on both platforms I suggest removing WARNING here and just saying "no symlinks were found in the Bazel output directory that needed hardening. Ignoring the error returned by the symlinks find command".
If the error is specific to windows, then we should only ignore the error on windows.
There was a problem hiding this comment.
Ah, you're right. find on linux just returns no files but 0 exit status. Fixed.
pkg/rbeconfigsgen/rbeconfigsgen.go
Outdated
| @@ -379,7 +379,7 @@ func installBazelisk(d *dockerRunner, downloadDir, execOS string) (string, error | |||
| } | |||
| defer resp.Body.Close() | |||
|
|
|||
| localPath := path.Join(downloadDir, filename) | |||
| localPath := filepath.Join(downloadDir, filename) | |||
There was a problem hiding this comment.
Curious: What errors do you see when using path.Join on Windows?
There was a problem hiding this comment.
I didn't actually see an error for this - I changed this because I kept getting "file not found" when trying to dir the tools folder... When in reality it was simply because there were no symlinks to find, hah.
In any case, I think using filepath (which respects \) should be safer?
There was a problem hiding this comment.
Nvm - PS > cmd /r /unix/style/path indeed doesn't work. I get errors eg Invalid Switch - "Users".
Running it directly from cmd seems to work, but IIUC the container could be using either cmd or PS.
I tweaked it so it only cleans uses windows filepath on the dir command so it doesn't potentially break eg bazel paths.
pkg/rbeconfigsgen/rbeconfigsgen.go
Outdated
| switch o.ExecOS { | ||
| case "windows": | ||
| out = "" | ||
| log.Printf("WARNING: %s%v", errMsg, err) |
There was a problem hiding this comment.
Not a warning because it's not really actionable. Change to Ignoring error indicating no symlinks were found in the Bazel output directory: %v
No need to include errmsg because it would be confusing in the logs
These non-exhaustive fixes address some of the more obvious problems: 1. "find" works differently on windows. 2. There may be no symlinks on the local configs.
These non-exhaustive fixes address some of the more obvious problems: