Fix issue 329

[ashug06]

Summary

This PR fixes issue #329 by making necessary updates to local.rs and formatting source.wdl.

Changes

• Modified wdl-engine/src/backend/local.rs: (describe the change if you understand it)
• Formatted wdl-lint/tests/lints/inconsistent-newlines/source.wdl to maintain consistency.

Notes

• Let me know if any further changes are needed!

[a-frantz]

please rewrite your PR description according to the PR template we wrote and ensure all the tests and checks are working locally

[peterhuene]

Hi @ashug06.

Unfortunately your changes don't address #329. The issue is that for containerized backends (such as the Docker backend), we need a way for it to chown the files inside of the output directory without elevating privileges, ideally only once as a "cleanup" step and not on every task execution.

The way miniwdl handles this is to start a minimal container and perform the chown operation from within that container, which also runs as root and therefore the user running miniwdl doesn't need to sudo.

[peterhuene]

There should be no changes to the local backend to fix #329.

[peterhuene]

Likewise, this file should not have any changes; I suspect your git or editor are automatically changing the line endings in this file, but it is intentionally using a mix of new lines to test a lint.

If this is coming from git, I suspect we are missing a .gitattributes in the test directory to ignore it.

[Pabsthegreat]

yes it happened to me as well

[peterhuene]

I put up #338 to exclude the test from autocrlf behavior, which I suspect is the problem here.

[ashug06]

Hi @peterhuene,

Thank you for your feedback! I now understand that the required fix should involve handling chown inside the containerized backend rather than modifying local.rs. I appreciate the reference to miniwdl's approach, and I'll look into implementing a similar solution.
For now, I’ll revert the unintended changes to source.wdl and start working on the correct implementation. It might take me some time to complete, but I'll keep you updated on my progress.
Let me know if there are any additional suggestions or resources that could help!

[ashug06]

Hey @peterhuene, I'm trying to build the project, but I'm getting an error because the nodes-api branch is missing in bollard. Could you clarify which version or branch I should use?

[peterhuene]

Hi @ashug06.

That branch was deleted in favor of updating the patch in Cargo.toml to the upstream repository now that the fix was merged.

To get things building again, rebase your branch off of stjude-rust-labs/wdl's main branch.

[ashug06]

Hi @peterhuene, based on your feedback, I have implemented a chown_output_directory function to change the ownership of the output directory inside the container before removal. I plan to call this function before remove_container() in the relevant places.

Does this approach align with what you had in mind, or would you suggest any modifications?

[ashug06]

Hi @peterhuene, I have updated remove_container to call chown_output_directory before removal. Let me know if any further modifications are needed!

[peterhuene]

Hi @ashug06. I don't see your changes to the docker backend that I was expecting. Is your local branch pushed to your remote repo?

[peterhuene]

We should not have a bollard submodule.

What patches to bollard did you need to implement this PR? bollard itself should have everything we need to make this PR work (we also only use bollard transitively through crankshaft).

My expectations around an implementation here is that the docker backend in wdl-engine gets called once at the end of evaluation to perform a "cleanup" operation, and the cleanup operation for the docker backend would be to run a task that does a recursive chown on the entire output directory (thus we only ever do this once per run).

[peterhuene]

This PR shouldn't have a git file.

[ashug06]

Hi Peter,
Thanks for the review! I’ll check why my changes to the Docker backend aren’t visible and ensure they are pushed properly.

Regarding the bollard submodule, I didn’t intend to introduce it as a submodule—I'll remove it if it's unnecessary. My modifications were focused on implementing the cleanup operation, but I’ll revisit how bollard is used via crankshaft.

For the cleanup operation, I now understand that it should be executed once at the end of evaluation to recursively chown the entire output directory. I’ll adjust my implementation accordingly.

I also noticed the accidental inclusion of a .git file—I'll remove it in the next update.

Thanks for the guidance! I’ll update the PR soon.

[ashug06]

I noticed there's no existing Docker backend in wdl-engine/src/backend/. I created docker.rs with a cleanup function that runs chown -R inside a minimal container at the end of execution. Is this approach correct?

[peterhuene]

Duplicating from my slack message:

The docker backend is implemented via src/crankshaft.rs (crankshaft is the library that's abstracting task execution and will support more than just docker soon). We should probably add a cleanup method to the TaskExecutionBackend trait that is called whenever the engine finishes top-level evaluation (upon success or failure) and the crankshaft backend, if configured to use Docker, should perform the chown