Skip to content

feat: Improve exec error handling#1368

Merged
rickeylev merged 4 commits intobazel-contrib:mainfrom
chrislovecnm:update-which
Aug 10, 2023
Merged

feat: Improve exec error handling#1368
rickeylev merged 4 commits intobazel-contrib:mainfrom
chrislovecnm:update-which

Conversation

@chrislovecnm
Copy link
Copy Markdown
Contributor

@chrislovecnm chrislovecnm commented Aug 7, 2023

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


# Locate xcode-select
xcode_select = rctx.which("xcode-select")
if not xcode_select.exists:
Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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 :)

@chrislovecnm chrislovecnm force-pushed the update-which branch 2 times, most recently from 3a1ee5d to 3b8a47b Compare August 8, 2023 21:07
@@ -0,0 +1,32 @@
# Copyright 2023 The Bazel Authors. All rights reserved.
Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Collaborator

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.
Copy link
Copy Markdown
Collaborator

@rickeylev rickeylev left a 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.
Copy link
Copy Markdown
Collaborator

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
@rickeylev rickeylev enabled auto-merge August 10, 2023 18:02
Add missing parentheses to fix syntax error
@rickeylev rickeylev added this pull request to the merge queue Aug 10, 2023
Merged via the queue into bazel-contrib:main with commit 99695ee Aug 10, 2023
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.

Getting Started instructions don't work

2 participants