0xPolygonMiden / miden-base

Core components of the Polygon Miden rollup
MIT License
73 stars 45 forks source link

fix: Avoid writing on docs pipeline #970

Closed igamigo closed 5 days ago

igamigo commented 1 week ago

Looks like the docs were failing to build for miden-client due to the same error we have seen before, related to the pipeline's filesystem being read-only: https://docs.rs/crate/miden-client/0.6.0/builds/1507411. The problem seems to come specifically from miden-lib, but I also added an early return for the prover service as well since that would also likely fail.

igamigo commented 1 week ago

I see. We had tried to build the image and run everything but it was taking too long, so we went with a familiar approach (this problem came up some months ago). I'll give it a second try and test your approach (wasn't keeping in mind that we should be able to write to OUT_DIR). Thanks!

igamigo commented 1 week ago

Hmm, I spent some time trying to run the docs.rs pipeline (with the Docker alternative that they provide for MacOS instead of running it natively), but was not able to. I was able to sort through a couple minor issues but the image does not build correctly. Specifically, it fails to build the dummy crate with "failed to add essential files".

I decided to not waste too much more time on that but test your suggested approach anyway. I think this works in general, although unless I misunderstood something, these files specifically (kernel_v0.rs for example) did not seem to be the reason why the docs were failing to build according to the logs you pasted. It seems the main problem there was that with these changes, the kernel and note scripts were not being built (because the docs.rs check was being done before those) and so doing include_bytes!(concat!(env!("OUT_DIR"), "P2ID.masb")) would fail.

The main consequence of your approach is that code becomes a bit harder to browse (and less flexible in general) due to the include!()s, and files not being committed. So I guess for any build script, this could be an alternative:

1 - Build any binary file to env!("OUT_DIR") that we are including with include_bytes!() 2 - Check whether we are on the docs.rs pipeline and return if so 3 - Build any .rs file that we want to place under /src then. These files get committed to the source tree anyway

bobbinth commented 1 week ago

I would actually prefer to check in all generated files. We do this in other repos (e.g., miden-node for Rust files generated from .proto files) and it seems to be OK. So, I'm thinking we can do the following:

igamigo commented 6 days ago

I changed the approach back to what it was earlier, but only avoiding to build the Rust files specifically. I also moved the kernel error generation to miden-lib as suggested by Bobbin, and tweaked the CI check.

bobbinth commented 6 days ago

One other thing: maybe we should merge this into main? This way, I could make a patch release and this should allow docs to be built on docs.rs.

igamigo commented 5 days ago

@bobbinth if you agree with testing whether the error that Philipp reported does not happen with a patch release we can go ahead and merge this

bobbinth commented 5 days ago

@bobbinth if you agree with testing whether the error that Philipp reported does not happen with a patch release we can go ahead and merge this

Yes, let's merge. I'll be able to do a patch release later today.

bobbinth commented 5 days ago

I published v0.6.2 crates and it seems like miden-lib docs are now building fine.