-
Notifications
You must be signed in to change notification settings - Fork 539
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
feat: Improve exec error handling #1368
Conversation
@@ -110,6 +110,8 @@ def _get_xcode_location_cflags(rctx): | |||
|
|||
# Locate xcode-select | |||
xcode_select = rctx.which("xcode-select") | |||
if not xcode_select.exists: |
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.
According to the docs, which() returns None if the binary isn't found. Are you find otherwise? That it returns a path object, but exists is False?
https://bazel.build/rules/lib/builtins/repository_ctx#which
Returns the path of the corresponding program or None if there is no such program in the path.
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 will update and test
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 created a function to handle all the which calls, and I am now testing for None. I moved uname
to uname-hide
and it works like a champ :)
3a1ee5d
to
3b8a47b
Compare
@@ -0,0 +1,32 @@ | |||
# Copyright 2023 The Bazel Authors. All rights reserved. |
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 would have put this in util. But we don't have skylib imported everywhere.
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.
Nah, that's good. It's a good practice to avoid mixing code that is only used in (or can only be used in) the repo, loading, and analysis phases, for exactly this sort of reason.
If you want to move this into a new e.g. repo_util.bzl, I'd be +1 to that.
At times binaries are not in the path. This commit uses a new method to test that a binary exists before we try to execute the binary. The method fails with a message if the binary does not exist.
3b8a47b
to
83b9b4b
Compare
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.
Just nits really.
@@ -0,0 +1,32 @@ | |||
# Copyright 2023 The Bazel Authors. All rights reserved. |
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.
Nah, that's good. It's a good practice to avoid mixing code that is only used in (or can only be used in) the repo, loading, and analysis phases, for exactly this sort of reason.
If you want to move this into a new e.g. repo_util.bzl, I'd be +1 to that.
Add quotes around command name
Pull fail() call out into its own line/block
Add missing parentheses to fix syntax error
At times binaries are not in the path. This commit tests that the binary exists before we try to execute the binary.
This allows us to provide a more informative error message to the user.
Closes: #662