Skip to content

feat: support configuring the manifold URL #968

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

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

Conversation

lf-
Copy link
Contributor

@lf- lf- commented May 28, 2025

I have a highly trivial Manifold implementation internally at Mercury for which I would like to set up for buck2 to receive rage and all that at. This allows for fully implementing the build logs feature mentioned in #441.

There's still a buck2_core::facebook_only() or two that need removed and some more tidying.

@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 May 28, 2025
@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. (Because this pull request was imported automatically, there will not be any future comments.)

@lf- lf- force-pushed the jadel/dux-3504-locally-euclidean branch 2 times, most recently from d99291a to 236e347 Compare May 29, 2025 00:59
@alexlian alexlian added enhancement New feature or request UX User Experience labels May 30, 2025
@iguridi
Copy link
Contributor

iguridi commented May 30, 2025

This is neat, taking a look

Copy link
Contributor

@iguridi iguridi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing missing here is that it won't work when downloading logs via --trace-id .

Probably the way forward on that is to actually use our ManifoldClient there instead of relying on shelling out. Having said that, this PR should still work for rage and html, so if you just want to add a TODO there and do it in a different PR that is ok with me

added a couple of nits, feel free to ignore those for now

@lf-
Copy link
Contributor Author

lf- commented Jun 4, 2025

One thing missing here is that it won't work when downloading logs via --trace-id .

It actually does work for this, but only because I simply implemented the curl URL structure in locally-euclidean. It is a little goofy but it works.

How would you like to proceed with fixing this after landing this PR though? Should buck2 just fetch them from the fbcdn (and equivalent) URLs in ManifoldClient? Should it implement the actual Manifold GET API (in which case I can't help you because I don't know what it is)?

@lf- lf- force-pushed the jadel/dux-3504-locally-euclidean branch 3 times, most recently from 2adf680 to f2d09c4 Compare June 4, 2025 22:48
@lf-
Copy link
Contributor Author

lf- commented Jun 5, 2025

Looks like a bogus CI failure (or at least one I have nothing to do with). Surprised it builds internally at Facebook given that I suspect I broke a usage of the API I replaced (ManifoldClient::new), but shrug! Whatever works!

@lf- lf- requested a review from iguridi June 5, 2025 19:04
I have a highly trivial Manifold implementation internally at Mercury
(https://github.com/mercurytechnologies/locally-euclidean) for which I would like to set
up for buck2 to receive rage and all that at. This allows for fully implementing the build logs
feature mentioned in facebook#441.

There's still a buck2_core::facebook_only() or two that need inspected/removed and some more tidying.
@lf- lf- force-pushed the jadel/dux-3504-locally-euclidean branch from f2d09c4 to 8f07fe6 Compare June 16, 2025 17:06
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. enhancement New feature or request UX User Experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants