Defer file reading until package build time about rpm HOT 9 OPEN

@drahnr @cmeister2 Does this sound OK to you? Can you see any problems with the idea?

from rpm.

My main issue with the outlined approach, is the fact that we're not able to provide a reader but make it internal to the fn build(..). So we hard code files iiuc, which I'd rather not.

We don't necessarily have to read the files when passed to with_file or with_file_async, but could defer the reading to the build invocation, while still maintaining passing impl BufRead (or it's async equiv).

from rpm.

Sure, I'm not tied strongly to specific implementation details, so long as we're not storing the entire contents of files in buffers in the builders.

Could you elaborate on this aspect, I'm not sure I understand what you mean.

we're not able to provide a reader but make it internal to the fn build(..). So we hard code files iiuc, which I'd rather not.

from rpm.

My assumption was you wanted to use std::fs::File internally.

from rpm.

If by internally you mean on the builder struct then no, I just want to store the path, so that build() (or presumably build_async()) can use whatever method it wants to read the files.

Instead of storing a collection of eagerly-processed RPMFileEntrys inside the builder, we should probably try to store only the filename and RPMFileOptions

from rpm.

No immediate issue with the design. If I squinted enough with a security hat on I could potentially see an issue with storing a path and then reading from that path at a later time, if there was a sufficient gap between storing and then reading (and if that file can be replaced in the meantime), but I don't think that's a sufficient worry.

from rpm.

If by internally you mean on the builder struct then no, I just want to store the path, so that build() (or presumably build_async()) can use whatever method it wants to read the files.

Instead of storing a collection of eagerly-processed RPMFileEntrys inside the builder, we should probably try to store only the filename and RPMFileOptions

That's what I meant, you store the path, but then you already looked into using File in fn build().

from rpm.

That's what I meant, you store the path, but then you already looked into using File in fn build().

In theory you would have a build() and a build_async() (etc.) the same way there is currently with_file_async(). But that is necessary even if you were to pass impl BufRead or the async equivalent because the act of doing the reads is what needs to be async. Also, since the interfaces are different, that approach would pollute the RpmBuilder with additional complexity.

(Separately) I'm still not sure I appreciate the point of async for file IO though.

• Gets delegated to synchronous calls running inside a threadpool anyway
• You probably don't want to build or parse RPMs in an async context where you care about latency (e.g. webserver), because calculating / verifying digests, signatures, and particularly level-19 zstd compression (!!!) is computationally heavy, and the long computational events effectively cause "blocking". This is especially true for any single-threaded async runtime.
• Ergonomically speaking it's not that much of an improvement over spawn_blocking(|| ...)

On point 2, I don't have hard data on this, and I should probably do some profiling so that I'm basing this on evidence. I just want to call out that the tokio docs suggest not doing anything compute-bound inside an async runtime, and I have a feeling this may qualify.

from rpm.

fn build_async(..) could just consume a threadpool on which the packaging is placed from an execution perspective.

My main concern is the fact hardcoding std::path::Path pointing to local filesystem paths into the API, I'd like to see impl BufRead/impl Read being the only bound for file contents (or their async variants).

I also believe, we should consider splitting the sync and async builder to some extent, or provide a splitting function.

from rpm.