Skip to content

Fix a potential TypeException caused by None type - #668

Merged
aiuto merged 1 commit into
bazelbuild:mainfrom
sqybi:fix
Feb 15, 2023
Merged

aiuto merged 1 commit into
bazelbuild:mainfrom
sqybi:fix

Conversation

@sqybi

@sqybi sqybi commented Feb 10, 2023 •

Copy link
Copy Markdown
Contributor

os.access(content_path, os.X_OK) will raise a TypeException when content_path is None.
f_mode is only used when content_path is neither None nor empty, so I decided to move the assignment into this else-branch.

Thanks @mutongx for reporting this issue.

@sqybi
sqybi requested review from aiuto and nacl as code owners February 10, 2023 03:17
@google-cla

google-cla Bot commented Feb 10, 2023

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@aiuto aiuto self-assigned this Feb 10, 2023
@aiuto

aiuto commented Feb 10, 2023

Copy link
Copy Markdown
Collaborator

Sigh. The presence of this PR means that we don't have an adequate test case for this situation.
Do you have time to add one?

@aiuto aiuto left a comment

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.

I'll take the lack of response about adding a test as a "no".

@aiuto
aiuto merged commit 23a3cb4 into bazelbuild:main Feb 15, 2023
@sqybi

sqybi commented Feb 15, 2023

Copy link
Copy Markdown
Contributor Author

Really busy these days. I will try to add some tests on the weekend.

@sqybi
sqybi deleted the fix branch February 16, 2023 05:45
sqybi added a commit to sqybi/rules_pkg that referenced this pull request Feb 17, 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.

2 participants