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

Support 'file' scheme for external tool downloads #11295

Closed
gatesn opened this issue Dec 10, 2020 · 9 comments · Fixed by #11499
Closed

Support 'file' scheme for external tool downloads #11295

gatesn opened this issue Dec 10, 2020 · 9 comments · Fixed by #11499
Assignees

Comments

@gatesn
Copy link

gatesn commented Dec 10, 2020

It would be great if this worked, so I could pull these tools off a shared filesystem.

[scc]
url_template = "file:/path/to/scc"
@stuhood
Copy link
Member

stuhood commented Dec 10, 2020

We should absolutely do this: thanks for the report!

Aside from adjusting validation, I believe that the implementation would look like adding a conditional to the body of the rule that fetches the tool:

digest = await Get(Digest, DownloadFile, request.download_file_request)
extracted_archive = await Get(ExtractedArchive, Digest, digest)
return DownloadedExternalTool(extracted_archive.digest, request.exe)
... to either await Get(Digest, DownloadFile(..)), or await Get(Snapshot, PathGlobs([f'{relative_file_path}'])) (and then validate that the Snapshot captured a single file). This would have some downsides though: it would only support relative file URLs, and it would not be able to validate the file digest (currently).

A better implementation though might be to adjust the rust code to support both schemes:

let file_name = url
.path_segments()
.and_then(Iterator::last)
.map(str::to_owned)
.ok_or_else(|| format!("Error getting the file name from the parsed URL: {}", url))?;
let path = RelativePath::new(&file_name).map_err(|e| {
format!(
"The file name derived from {} was {} which is not relative: {:?}",
&url, &file_name, e
)
})?;
let maybe_bytes = core.store().load_file_bytes_with(digest, |_| ()).await?;
if maybe_bytes.is_none() {
DownloadedFile::download(core.clone(), url, file_name, digest).await?;
}
core.store().snapshot_of_one_file(path, digest, true).await
... because there is an associated Digest to validate, we know that we do not need to "watch" the file path, and can simply use rust's file API to capture it.

@Eric-Arellano
Copy link
Contributor

That could be a frustrating limitation that the file must be relative to the build root. Often, binaries like scc are installed in a more global location.

Instead, I suspect we'll want to add back proper support for PathGlobsAndRoot - I think there's an issue tracking it, as we need the functionality already and hack around it by cheating with Path().

@stuhood
Copy link
Member

stuhood commented Dec 10, 2020

@Eric-Arellano : Had the same thoughts. Updated my comment.

@jsirois
Copy link
Contributor

jsirois commented Dec 10, 2020

If the file: support required size and hash just like the https?: support then there would be no need for path globs, etc - right? In fact, if reqwest supported file: urls then this would all just work.

@Eric-Arellano
Copy link
Contributor

Beyond how to specify the "URL", another complication is what that URL can point to. Atm, users have no way to change the generate_exe() path through the options system - this method defines how to locate the binary within the downloaded file, e.g. ./bin/scc or ./pex. So, you could not point to, say, an SCC installed via apt-get, as it would not be the zip file we expect.

I think the solution is simple: add --exe-template and --exe-platform-mapping options to TemplatedExternalTool. @thamenato and I did not do that originally to avoid new complexity, but it could be helpful even without this file scheme added. It would allow you to host a file/archive with a different structure than the default.

@stuhood
Copy link
Member

stuhood commented Dec 10, 2020

If the file: support required size and hash just like the https?: support then there would be no need for path globs, etc - right? In fact, if reqwest supported file: urls then this would all just work.

Yep, exactly. I had updated my comment to reflect that. I expect that doing this directly in the DownloadFile intrinsic on the rust side is the way to go.

@Eric-Arellano
Copy link
Contributor

another complication is what that URL can point to...I think the solution is simple

@gatesn let me know if you'd be interested in adding this change, and I'd be happy to write up some instructions. With that added, the only blocker is the Rust changes Stu is mentioning. If not, I'll try to add it late next week.

@gatesn
Copy link
Author

gatesn commented Dec 10, 2020

Does rust not support opening arbitrary URLs? E.g. Java and Python allow globally registering support for a scheme.

urllib.open for instance works with http or file, and probably a bunch of other things.

@jsirois
Copy link
Contributor

jsirois commented Dec 10, 2020

Does rust not support opening arbitrary URLs? E.g. Java and Python allow globally registering support for a scheme.

urllib.open for instance works with http or file, and probably a bunch of other things.

We use the Reqwest library and it does not. A feature request is outstanding though: seanmonstar/reqwest#178

@jsirois jsirois self-assigned this Jan 27, 2021
jsirois added a commit to jsirois/pants that referenced this issue Jan 27, 2021
Fixes pantsbuild#11295

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
jsirois added a commit to jsirois/pants that referenced this issue Jan 28, 2021
Fixes pantsbuild#11295

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
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 a pull request may close this issue.

4 participants