Downloading Binaries #3575
Downloading Binaries #3575
Conversation
zuperzane
requested review from
Andrewyx,
sauravbanna and
williamckha
as code owners
Andrewyx
left a comment
Andrewyx
left a comment
There was a problem hiding this comment.
Nice work so far! Left some comments
| url = "https://api.github.com/repos/UBC-Thunderbots/Software/releases" | ||
|
|
||
| headers = {"Accept": "application/vnd.github+json"} |
There was a problem hiding this comment.
We should probably move this to the constants class
| for release in releases: | ||
| for asset in release.get("assets", []): | ||
| binaries.append( | ||
| { | ||
| "name": asset["name"], | ||
| "download_url": asset["browser_download_url"], | ||
| "size_bytes": asset["size"], | ||
| "created_at": asset["created_at"], | ||
| "release_tag": release["tag_name"], | ||
| } | ||
| ) | ||
| if len(binaries) == 5: | ||
| break | ||
| if len(binaries) == 5: | ||
| break |
There was a problem hiding this comment.
This is grabbing 5 binaries right? Each release should only have 1 binary for each platform (i.e. we should not grab binaries which do not match the client's platform).
Also 5 here is a magic number so ideally we should make it a constant
| url = "https://api.github.com/repos/UBC-Thunderbots/Software/releases" | ||
| headers = {"Accept": "application/vnd.github+json"} |
There was a problem hiding this comment.
nit: see above const comment
src/software/thunderscope/binary_context_managers/runtime_installer.py
Outdated
Show resolved
Hide resolved
src/software/thunderscope/binary_context_managers/runtime_installer.py
Outdated
Show resolved
Hide resolved
| # Ensure target directory exists | ||
| subprocess.run(["sudo", "mkdir", "-p", str(target_dir)], check=True) |
There was a problem hiding this comment.
This looks like shared functionality (checking if the external runtimes dir exists + creating it) between this PR and #3569. We should move this operation to the superclass
src/software/thunderscope/binary_context_managers/runtime_installer.py
Outdated
Show resolved
Hide resolved
src/software/thunderscope/binary_context_managers/runtime_installer.py
Outdated
Show resolved
Hide resolved
src/software/thunderscope/binary_context_managers/runtime_installer.py
Outdated
Show resolved
Hide resolved
| headers = {"Accept": "application/vnd.github+json"} | ||
|
|
||
| response = requests.get(url, headers=headers) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
We should log this error, then default to the local runtimes instead. This would be handy given there are many times we might lose wifi unknowingly
src/software/thunderscope/binary_context_managers/runtime_installer.py
Outdated
Show resolved
Hide resolved
src/software/thunderscope/binary_context_managers/runtime_installer.py
Outdated
Show resolved
Hide resolved
|
|
||
| for release in releases: | ||
| for asset in release.get("assets", []): | ||
| if asset["browser_download_url"].endswith(TARGET_SUFFIX): |
There was a problem hiding this comment.
yeah, agree with storing the last looked up binaries and checking within that before, before making the API call to fetch releases again
src/software/thunderscope/binary_context_managers/runtime_installer.py
Outdated
Show resolved
Hide resolved
|
|
||
| for release in releases: | ||
| for asset in release.get("assets", []): | ||
| if asset["browser_download_url"].endswith(TARGET_SUFFIX): |
There was a problem hiding this comment.
also, the fetching code here is similar to the code block in fetch_remote_runtimes. could probably extract the API call logic into a new method that just returns the releases json, and each method can filter accordingly
|
|
||
| else: | ||
| subprocess.run( | ||
| ["cp", str(archive_path), str(INSTALL_DIR / filename)], |
There was a problem hiding this comment.
archive_path and INSTALL_DIR are not defined, I think you meant tmp_path and target_dir
nycrat
left a comment
nycrat
left a comment
There was a problem hiding this comment.
looks good, works on my local. left a few nits but perhaps we should just get this merged asap
| # I'm going to assume you are trying to reload the assets so reset the download_urls | ||
| if len(self.download_urls) != 0: | ||
| self.download_urls = [] |
There was a problem hiding this comment.
nit: not necessary, the fetch_remote_runtimes function only runs once
| url = RuntimeManagerConstants.INSTALL_URL | ||
| headers = {"Accept": "application/vnd.github+json"} | ||
|
|
||
| response = requests.get(url, headers=headers) | ||
| logger.warning(response) | ||
|
|
||
| releases = response.json() |
| for i in range(len(self.download_urls)): | ||
| if self.download_urls[i].endswith(TARGET_SUFFIX): | ||
| selected_asset = self.download_urls[i] |
There was a problem hiding this comment.
nit: I would just do download_url in self.download_urls instead of a range-based loop
| INSTALL_URL = "https://api.github.com/repos/UBC-Thunderbots/Software/releases" | ||
| DOWNLOAD_PREFIX = "https://github.com/UBC-Thunderbots/Software/releases/download/" |
There was a problem hiding this comment.
nit: perhaps change to RELEASES_URL, and DOWNLOAD_URL
williamckha
dismissed
sauravbanna’s stale review
eric will clean the pr up later
5 participants
Description
Gathers list of names of binaries to download and downloads them and unzips to external runtime directory
Testing Done
Can test with UI I believe
Resolved Issues
resolves #3559
Length Justification and Key Files to Review
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue