Enhance utoipa-swagger-ui build #936
Conversation
|
Follow up for #923 @nightkr @oscar6echo |
juhaku
force-pushed
the
chore-swagger-ui-build-enhancements
branch
2 times, most recently
from
5e72315
to
62e5264
Compare
Use `curl` to download Swagger UI by default and allow using `reqwest` optionally with `reqwest` feature flag. Reqwest will be automatically enabled on Windows targets. Change the re-build parameters as follows. * For file protocol the re-build should happen when the upstream file changes * For http protocol the re-build should happen when the environment variable holding the downloadd URL will change.
juhaku
force-pushed
the
chore-swagger-ui-build-enhancements
branch
from
62e5264
to
c54e1ab
Compare
|
Thx for refining it further 👍 |
| reqwest = ["dep:reqwest"] | ||
| url = ["dep:url"] |
There was a problem hiding this comment.
I don't see anything feature-flagged on url, so it looks like it's just always required?
There was a problem hiding this comment.
I think there are some places like here.
But in principle at least, url is necessary to parse SWAGGER_UI_DOWNLOAD_URL in all cases except in the case of vendored swagger_ui.zip so maybe the vendored flag is enough ?
There was a problem hiding this comment.
True, would be enough at this point, and would work without explicit url feature since the optional dependencies becomes implicit feature flags anyways. But it does not matter.
| // with file protocol utoipa swagger ui should compile when file changes | ||
| println!("cargo:rerun-if-changed={:?}", file_path); | ||
|
|
||
| println!("start copy to : {:?}", zip_path); | ||
| fs::copy(file_path, zip_path.clone()).unwrap(); |
There was a problem hiding this comment.
At this point this really just seems equivalent to zip_path = file_path;.
There was a problem hiding this comment.
Yeah, while this is little bit older commit but in practice yes, the copy seems to be superfluous since it is anyway extracted to the OUTPUT dir. But perhaps we can live with that 😆
|
|
||
| let url = | ||
| env::var("SWAGGER_UI_DOWNLOAD_URL").unwrap_or(SWAGGER_UI_DOWNLOAD_URL_DEFAULT.to_string()); | ||
| env::var(SWAGGER_UI_DOWNLOAD_URL).unwrap_or(SWAGGER_UI_DOWNLOAD_URL_DEFAULT.to_string()); |
There was a problem hiding this comment.
url is parsed into url::Url on every branch taken (explicitly for file://, explicitly for curl, and implicitly via IntoUrl for reqwest).
Doing it up here instead would also give access to Url::schema instead of approximating it with str::starts_with.
There was a problem hiding this comment.
True that. Maybe something that can be considered in future if it starts to bother someone 😆 Not going to edit it for now though
| let zip_filename = url.split('/').last().unwrap().to_string(); | ||
| let zip_path = [&target_dir, &zip_filename].iter().collect::<PathBuf>(); | ||
|
|
||
| if !zip_path.exists() { |
There was a problem hiding this comment.
I assume the cache is still desired for HTTP(S) downloads.
There was a problem hiding this comment.
Yes: I understand that this is covered by println!("cargo:rerun-if-env-changed={SWAGGER_UI_DOWNLOAD_URL}"); here, right ?
There was a problem hiding this comment.
Sort of, rerun-if-env-changed tells cargo about more reasons to rebuild, while the old cache was trying to avoid redownloading when it was rebuilt. (On the other hand, I'm not sure how useful that was anyway given that I'm not sure how Cargo decides on when to recreate out folders.)
There was a problem hiding this comment.
To be clear: I'm fine with the changed behaviour, just wanted to double check that it was intended.
There was a problem hiding this comment.
Yeah, I removed it just because it appears to be not consistent on creating the folders thus even if there was the cache form previous builds, cargo might still create new set of folders when building thus invalidating the cache in a mean time as well. Perhaps just better of to let it download it when ever cargo should rebuild it. After all that is not going to be build unless one runs cargo clean.
Use
curlto download Swagger UI by default and allow usingreqwestoptionally withreqwestfeature flag. Reqwest will be automatically enabled on Windows targets.Change the re-build parameters as follows.