Fix downloading URLs with invalid characters #5921
Conversation
OpamDownload.download downloads a given URL using the basename of the URL as the filename. On Windows, where are there are many restrictions on valid filenames, this causes a problem if the URL includes any query string. Since OpamDownload passes the name used to the continuation, on Windows the illegal characters are simply replaced with underscores.
There was a problem hiding this comment.
looks fine on its own, however I'm wondering how things interact if two files (e.g. extra-files) are downloaded and rewritten to the same name.
For example file*.txt and file?.txt would both rewrite to file_.txt and would overwrite each other. Is this detected by OpamDownload?
If so it would be nice to have a testcase for this, as well as for the base fix.
|
It's worth the test case regardless, but this isn't disambiguating the final name - it's the intermediate name which if I traced the code through correctly is always done in an empty temporary directory. For example, in: the final name ( |
|
oh i see! Then I'm wondering, does the name have to be similar as the original? Why not simply have one static name that's always going to be the same? |
| let dst = | ||
| OpamFilename.(create dstdir (Base.of_string (OpamUrl.basename url))) | ||
| OpamFilename.(create dstdir (Base.of_string base)) |
There was a problem hiding this comment.
I'm wondering if we should have that check/change in OpamFilename.Base directly, like that we are sure that every file created is valid. But can we have that change (forbidden char -> _) for all files?
There was a problem hiding this comment.
I don't think we can ever automatically change it, but we may want to do things to cause better error messages. For example, if an opam file actually specifies extra-source "zstd-detection?full_index=1.patch" { then I don't think opam should ever do anything to make this work, but it perhaps fail with a better error message if this is encountered.
There was a problem hiding this comment.
(the key point here is that this filename is internal to opam)
| let dst = | ||
| OpamFilename.(create dstdir (Base.of_string (OpamUrl.basename url))) | ||
| OpamFilename.(create dstdir (Base.of_string base)) |
There was a problem hiding this comment.
In opam code, there is no collision with _ change (as you pointed), but I'm wondering if we could have collision for library users.
There was a problem hiding this comment.
Possibly - on the plus-side, the fallout is limited to Windows, where the code would have been failing before!
Co-authored-by: R. Boujbel <rjbou@ocamlpro.com>
OpamDownload.downloaddownloads a given URL using the basename of the URL as the filename. On Windows, where are there are many restrictions on valid filenames, this causes a problem if the URL includes any query string. Since OpamDownload passes the name used to the continuation, on Windows the illegal characters are simply replaced with underscores.