Skip to content

Expand home path in fs.PathExists#3270

Merged
toastwaffle merged 2 commits intothought-machine:masterfrom
cezarelnazli:expand-home-lstat
Oct 10, 2024
Merged

Expand home path in fs.PathExists#3270
toastwaffle merged 2 commits intothought-machine:masterfrom
cezarelnazli:expand-home-lstat

Conversation

@cezarelnazli
Copy link
Copy Markdown
Contributor

A build rule like this

genrule(
    name = name,
    secrets = ["~/test.yaml"],
)

fails building with this error

failed to calculate hash: Attempting to record rule hash: cannot calculate hash for ~/test.yaml: file does not exist

The hash functions use os.Lstat to check for file existence, and lstat(2) does not seem to expand the home path itself.

So this PR calls the ExpandHomePath before os.Lstat in the functions which check file presence. I've tested it by bootstrapping please on this branch and building the rule that initially failed.

Not sure if this is the best approach, if there are better suggestions I'm happy to look into it.

A build rule like this

```python
genrule(
    name = name,
    secrets = ["~/test.yaml"],
)
```

fails building with this error

```
failed to calculate hash: Attempting to record rule hash: cannot calculate hash for ~/test.yaml: file does not exist
```

The hash functions use `os.Lstat` to check for file existence, and
`lstat(2)` does not seem to expand the home path itself.

So this PR calls the `ExpandHomePath` before `os.Lstat` in the functions
which check file presence. I've tested it by bootstrapping `please` on
this branch and building the rule that initially failed.
@cezarelnazli cezarelnazli marked this pull request as ready for review October 9, 2024 09:21
@toastwaffle
Copy link
Copy Markdown
Contributor

Hey, thanks for sending us a PR!

Looking at how ExpandHomePath is used elsewhere, I think this is the wrong approach, and we should be making sure we expand the path before we pass it into these functions (particularly because I don't think we want to allow srcs of build targets to be paths in a home directory).

Are you alright to chase that down?

@toastwaffle toastwaffle self-requested a review October 9, 2024 09:31
@cezarelnazli
Copy link
Copy Markdown
Contributor Author

Sounds good, I'll look into it, thanks!

@toastwaffle toastwaffle merged commit 53f05bb into thought-machine:master Oct 10, 2024
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