Skip to content
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

Add support to upload action results to an RE client. #771

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hugwijst
Copy link

@hugwijst hugwijst commented Sep 9, 2024

No description provided.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 9, 2024
@thoughtpolice
Copy link
Contributor

thoughtpolice commented Sep 9, 2024

Awesome, this looks like an even more thorough version of #764 which I filed a few days ago, and is a revive of an older PR, which I'm not sure you saw. Did you test it with symlinks? I'm happy to abandon my patch in favor of this once since it looks better at a glance anyway.

EDIT: I'm alternatively happy to port your changes into my patch if you'd like, but frankly I'm just hopeful some version of this patch goes in one way more another, soonish.

@thoughtpolice
Copy link
Contributor

See also #765 which is needed to fix stdout uploads in the local caching case.

@hugwijst
Copy link
Author

hugwijst commented Sep 9, 2024

Awesome, this looks like an even more thorough version of #764 which I filed a few days ago, and is a revive of an older PR, which I'm not sure you saw. Did you test it with symlinks? I'm happy to abandon my patch in favor of this once since it looks better at a glance anyway.

I did not see your version, nor did I try symlinks explicitly. I just implemented the converse of convert_action_result which was easier than I anticipated. I have some internal use case for which this seems to cache correctly, but don't have explicit tests.

EDIT: I'm alternatively happy to port your changes into my patch if you'd like, but frankly I'm just hopeful some version of this patch goes in one way more another, soonish.

I'm happy to go either way too, your PR looks like it has more comments 😅 I too would like to see this feature mainlined soonish.

@hugwijst
Copy link
Author

hugwijst commented Sep 9, 2024

See also #765 which is needed to fix stdout uploads in the local caching case.

Oh, interesting. I hadn't run into that yet, but good to know where to look when I do.

@hugwijst
Copy link
Author

@thoughtpolice, I just found another bug where buck2 ignores the preferred hashing algorithm when checking if it can upload to action cache, instead always using SHA1. See #784 for a fix.

@dmezh
Copy link

dmezh commented Nov 1, 2024

@JakobDegen Gentle bump on this one and #784

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants