-
Notifications
You must be signed in to change notification settings - Fork 96
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
Make rbe_configs_gen more windows friendly. #954
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Done.
} | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: What errors do you see when using path.Join
on Windows?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Done.
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: