Skip to content

[Build] Support windows in resolve_buck.py #4856

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 8, 2025
Merged

[Build] Support windows in resolve_buck.py #4856

merged 2 commits into from
Jan 8, 2025

Conversation

python3kgae
Copy link
Contributor

Add Windows support to BUCK_PLATFORM_MAP.
Also use urllib.request.urlretrieve directly to work around PermissionError on Windows.

For #4661

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Aug 23, 2024 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/4856

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit 86429ca with merge base 241cd0c (image):

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@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 Aug 23, 2024
python3kgae
python3kgae commented Aug 23, 2024
View reviewed changes
build/resolve_buck.py
print(f"Downloading buck2 from {buck2_archive_url}...", file=sys.stderr)
urllib.request.urlretrieve(buck2_archive_url, archive_file.name)
archive_file, _ = urllib.request.urlretrieve(buck2_archive_url)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is based on posit-dev/py-shiny#287

It works around PermissionError on Windows.
This happens because urlretrieve is trying to open an already-open temporary file by name, which isn't permitted on Windows. It's basically the same as

from tempfile import NamedTemporaryFile

with NamedTemporaryFile() as tmp:
    open(tmp.name, "wb")

@digantdesai digantdesai added module: build/install Issues related to the cmake and buck2 builds, and to installing ExecuTorch triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Aug 23, 2024
@python3kgae
Copy link
Contributor Author

Gentle ping.

@python3kgae
Copy link
Contributor Author

Don't have access to add more reviewer myself :(
Hope someone could see this.

@kirklandsign
@dbort
@tarun292
@dvorjackz

@python3kgae
Copy link
Contributor Author

Rebased and gentle ping again.

@mergennachin mergennachin mentioned this pull request Jan 7, 2025
Closed
@mergennachin
Copy link
Contributor

@python3kgae - sorry for the delayed response

we missed this PR.

Would you mind rebasing this and retest the flow? Happy to merge

@python3kgae
[Build] Support windows in resolve_buck.py
952624e 
Add Windows support to BUCK_PLATFORM_MAP.
Also use urllib.request.urlretrieve directly to work around PermissionError on Windows.

For pytorch#4661
@python3kgae
Copy link
Contributor Author

@python3kgae - sorry for the delayed response

we missed this PR.

Would you mind rebasing this and retest the flow? Happy to merge

Not a problem.
Rebased.

@mergennachin
Copy link
Contributor

@python3kgae

@swolchok just landed this PR (#7448), which updates the buck version altogether.

would you mind rebasing the PR and finding the appropriate updated hash version for windows for 2024-12-16 version instead?

@python3kgae
Copy link
Contributor Author

@python3kgae

@swolchok just landed this PR (#7448), which updates the buck version altogether.

would you mind rebasing the PR and finding the appropriate updated hash version for windows for 2024-12-16 version instead?

Done.

@mergennachin mergennachin added the release notes: build Changes related to build, including dependency upgrades, build flags, optimizations, etc. label Jan 8, 2025
@mergennachin mergennachin merged commit 1bac885 into pytorch:main Jan 8, 2025
68 of 69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mergennachin mergennachin mergennachin approved these changes

@GregoryComer GregoryComer Awaiting requested review from GregoryComer

@swolchok swolchok Awaiting requested review from swolchok

@tarun292 tarun292 Awaiting requested review from tarun292

@dbort dbort Awaiting requested review from dbort

@kirklandsign kirklandsign Awaiting requested review from kirklandsign

Assignees
No one assigned
Labels
ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. module: build/install Issues related to the cmake and buck2 builds, and to installing ExecuTorch release notes: build Changes related to build, including dependency upgrades, build flags, optimizations, etc. triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@python3kgae @mergennachin @digantdesai @facebook-github-bot