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

Add http & https asset sources #16366

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

mrchantey
Copy link
Contributor

@mrchantey mrchantey commented Nov 13, 2024

Objective

Solution

  • Add http & https asset sources

Testing

  • Verify visually by running cargo run --example http_source

Showcase

Bevy now supports assets loaded via url!

Add the http_source and optionally http_source_cache features.

  // Simply use a url where you would normally use an asset folder relative path
  let handle = asset_server.load("https://raw.githubusercontent.com/bevyengine/bevy/refs/heads/main/assets/branding/bevy_bird_dark.png");
  commands.spawn(Sprite::from_image(handle));

@mrchantey
Copy link
Contributor Author

It looks like surf and http-cache-surf have failed licence, vunerability & unmaintaned checks. Does that mean they're not allowed as first party dependencies?

@BenjaminBrienen
Copy link
Contributor

It's possible to add an exception for the licenses, but unmaintained and vulnerable is pretty bad.

@IQuick143 IQuick143 added C-Feature A new feature, making something new possible A-Assets Load files from disk to use for things like images, models, and sounds C-Dependencies A change to the crates that Bevy depends on X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 13, 2024
@mnmaita
Copy link
Member

mnmaita commented Nov 13, 2024

This could be done with something like reqwest maybe to avoid using the unmaintained crates. I successfully ran the example with it but my code is probably bad 😅. I can still push a PR as a showcase in case it serves as inspiration.

Copy link
Contributor

@MalekiRe MalekiRe left a comment

Choose a reason for hiding this comment

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

Yep this looks awesome to me! Also I wasn't aware of the surf crate; I'll have to keep it in mind for any bevy dashboards I make in the future

@mrchantey
Copy link
Contributor Author

I can still push a PR as a showcase in case it serves as inspiration.

@mnmaita actually that would be great, I've replaced surf and http-cache-surf with reqwest line for line but am a bit stuck at the moment with reqwest demanding to be run inside a tokio runtime.

// crates/bevy_asset/src/http_source.rs#134

    let client = reqwest_middleware::ClientBuilder::new(Client::new())
        .with(Cache(HttpCache {
            mode: CacheMode::Default,
            manager: CACacheManager::default(),
            options: HttpCacheOptions::default(),
        }))
        .build();

    let response = ContinuousPoll(client.get(str_path).send())
        .await

@jf908
Copy link
Contributor

jf908 commented Nov 15, 2024

FWIW surf has a bug where any erroring http response >8kb stops every subsequent request from succeeding and is unlikely to ever be fixed which made me unable to use bevy_web_asset so I would personally appreciate a switch 😄.

If you can't get reqwest to work perhaps hyper would suffice since it's already in the dep tree for BRP? Perhaps too low level for this.

@mrchantey
Copy link
Contributor Author

The council of bevy async wizards has spoken and we're going with ureq. It doesn't currently have a http-cache wrapper but I've opened an issue and the author might look into creating one when they have time. I guess its worth waiting a bit to see if ureq will get first party support before rolling our own http-cache wrapper, in which case this pr is ready for review :)

A few notes:

  • We're still failing check-advisories & check-licences
  • I used gh codespaces so wasnt able to cargo run --example http_source, can somebody please try running that?

Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@jf908
Copy link
Contributor

jf908 commented Nov 19, 2024

  • I used gh codespaces so wasnt able to cargo run --example http_source, can somebody please try running that?

Tested on Windows 11. The example prints a 404 error when the .meta file fails to load and doesn't show the sprite.

If I configure AssetPlugin to AssetMetaCheck::Never, it runs correctly.

I believe the asset is still supposed to load successfully even if the .meta file 404s so I think this is a bug?

@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 20, 2024
@BenjaminBrienen
Copy link
Contributor

We're still failing check-advisories & check-licences
I used gh codespaces

You can ignore that. It isn't a required check. Once those 2 small issues above are fixed, this should be in a pretty good state, I think.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Once there's a note or two about the license in the docs I'm happy to merge this.

@mockersf
Copy link
Member

I don't think it's worth it to merge this without https support

Nowadays everything is https. hiding the license behind a not enabled feature is not a fix, because for pretty much all use cases it will need to be enabled and the license issue solved

@mockersf mockersf added X-Controversial There is active debate or serious implications around merging this PR and removed X-Contentious There are nontrivial implications that should be thought through labels Dec 24, 2024
@cpu
Copy link

cpu commented Jan 4, 2025

It seems to have better licensing but worse devx, requiring non-rust dependencies for windows builds.

FWIW the situation here has improved since the linked discussions. Since v0.23.13 Rustls uses aws-lc-rs with the prebuilt-nasm feature to deliver equivalent Windows support as ring.

@mrchantey mrchantey added S-Blocked This cannot move forward until something else changes and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 6, 2025
@mrchantey
Copy link
Contributor Author

FWIW the situation here has improved since the linked discussions

Thats very helpful. it increasingly looks like aws-lc-rs is the path forward, im tagging this pr as blocked until the ureq authors make a call on an option to exclude ring.

@algesten
Copy link

algesten commented Jan 6, 2025

I made a PR in ureq that would allow it to be compiled without ring, and to pick any CryptoProvider with rustls. algesten/ureq#931

The only drawback is that rustls isn't semver 1.x, which means I can't make semver major version guarantees for ureq in this API.

@algesten
Copy link

algesten commented Jan 6, 2025

@mrchantey before we merge that PR, I just want to double check. We can't guarantee this API under semver for ureq, is that a blocker for you? I.e. hypothetically, if rustls breaks compatibility of CryptoProvider between current version 0.23 and a future 0.24, we will only bump ureq minor version. Would that be a problem?

@mrchantey
Copy link
Contributor Author

@algesten Thanks for clarifying, not a problem but good to keep in mind, I'll make a note of it in the Cargo.toml.

@algesten
Copy link

algesten commented Jan 6, 2025

Cool. Thanks! I merge!

@mrchantey
Copy link
Contributor Author

@algesten fyi im getting compile errors, not sure if i set the feature flags up correctly:

ureq = { git = "https://github.com/algesten/ureq", rev = "fd82f2bef3c2b2a8eaeaac9aae7ea06ffe7449d3", optional = true, default-features = false, features = [
  # "rustls",             # compiles ok
  "rustls-no-provider",   # breaks compilation
  "gzip",
  "json",
] }
rustls = { version = "0.23", optional = true, default-features = false, features = [
  "aws_lc_rs",
  "logging",
  "std",
  "tls12",
] }

"brotli",
ureq = { git = "https://github.com/algesten/ureq", rev = "fd82f2bef3c2b2a8eaeaac9aae7ea06ffe7449d3", optional = true, default-features = false, features = [
# "rustls", # compiles ok
"rustls-no-provider", # breaks compilation
Copy link

Choose a reason for hiding this comment

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

Looking at this now. I'll add it to the CI.

Copy link

Choose a reason for hiding this comment

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

@algesten
Copy link

algesten commented Jan 6, 2025

@mrchantey thanks. Fixing it now. See algesten/ureq#935

Also adding the case to CI so it doesn't regress.

@algesten
Copy link

algesten commented Jan 6, 2025

@mrchantey try 423aa8f05b30799129080e1bbe821ef067b50ed2

@mrchantey
Copy link
Contributor Author

@algesten awesome all working as expected 🙏

I'll leave the S-Blocked tag until ureq 3 is out, there are now just a couple of changes that will need review:

  • allow OpenSSL license
  • The use of once_cell to avoid rebuilding the agent for every request:
    static AGENT: once_cell::sync::Lazy<ureq::Agent> = Lazy::new(|| {
      // build agent
    });
    let response = AGENT.get("example.com")...

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Jan 7, 2025
@algesten
Copy link

algesten commented Jan 7, 2025

@mrchantey We published 3.0.0-rc5 if you want to have a proper crate dep while waiting for the real 3.0.0. The timeline was discussed here: algesten/ureq#920

This release reset the clock on having some "time of quiet" before going full out on 3.0.0, but there's nothing on the radar that needs fixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Dependencies A change to the crates that Bevy depends on C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Upstream bevy_web_asset, allowing assets loaded via http