search

rust-lang / rustwide

Execute your code on the Rust ecosystem.
Apache License 2.0
180 stars 44 forks source link

Add support for path-based patches #56

Closed regexident closed 2 years ago

regexident commented 3 years ago

Combined with a sandbox mount this allows for patching with a local file path, instead of a remote git repo branch.

pietroalbini commented 3 years ago

Thanks for the PR! I'm curious what's the use for this is. I'm more than happy to merge it, mostly wondering what prompted it :)

Also, there are a couple notes I have on the code changes:

regexident commented 3 years ago

Thanks for the PR! I'm curious what's the use for this is. I'm more than happy to merge it, mostly wondering what prompted it :)

The idea behind the addition is being able to run an experiment from your local working directory rather than having to have a remote branch on a git repo.

Also, there are a couple notes I have on the code changes:

  • It'd be nice if Rustwide handled creating the read-only mount point when a path-based patch is added. Right now the API could be confusing, as you have to put the path inside the container in the method call. Mounting it somewhere like /opt/rustwide/patch/{id} behind the scenes would greatly improve the API (you can get the /opt/rustwide with crate::cmd::ROOT_DIR).

That would indeed be a nice feature. What would you suggest to use as {id} here?

  • There doesn't seem to be a buildtest for this change. Adding one would make sure this feature doesn't regress.

I looked for tests for the existing branch-based patch, but couldn't find any and didn't know how to best test these.

I honestly don't know. I'm not all that familiar with Docker, tbh.

pietroalbini commented 3 years ago

Sorry for taking a while to respond!

That would indeed be a nice feature. What would you suggest to use as {id} here?

I'm fine with whatever, even a random string or a number. It just needs to be unique :)

I looked for tests for the existing branch-based patch, but couldn't find any and didn't know how to best test these.

This would require a buildtest. I'd create a dummy crate that requires a patch to compile, provide the patch in the buildtest and ensure the crate builds fine. If you need more pointers you can ping me on Zulip!

I honestly don't know. I'm not all that familiar with Docker, tbh.

Once you have the buildtest working you can easily test this by adding an inside_docker test that tries to execute the buildtest you wrote before.

regexident commented 2 years ago

@pietroalbini Took me a while to actually get to it but it's done now.

Thanks for the great pointers on what I'd need to change and where, btw. Much appreciated!

I wasn't sure what would be the best dummy dependency to use for patching, so used carols10cents' dependencies placeholder crate (as it is unlikely to go away and hasn't been yanked).

It makes for a bit unintuitive toml file though:

[dependencies]
dependencies = "0.1.0"

[patch.crates-io]
dependencies = { path = "./dependencies" }

Open for suggestions on a better dependency to use.

pietroalbini commented 2 years ago

You can use https://crates.io/crates/empty-library as an empty dependency.

regexident commented 2 years ago

Hah, I was sure the project would have something like that. Fixed.

regexident commented 2 years ago

The PR should be ready to merge now, I think @pietroalbini.

Skgland commented 2 years ago

The PR should be ready to merge now, I think @pietroalbini.

I think a changelog entry should be added under the unreleased subheading.

regexident commented 2 years ago

@Skgland Done. 🙂

regexident commented 2 years ago

@pietroalbini Is there anything else I can/have to do to have this merged and a release cut with it?

pietroalbini commented 2 years ago

Sorry for taking a long time to merge this, will release it soon!

pietroalbini commented 2 years ago

Released rustwide 0.15.0 with this change.

regexident commented 2 years ago

Thank you so much for this @pietroalbini!