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

Remove target locking #43

Closed
wants to merge 3 commits into from
Closed

Conversation

idelvall
Copy link
Member

@idelvall idelvall commented Feb 6, 2024

This PR removes the locking in target mount, and also introduces a single target mount as opposed to one per target as we had before.

  • Target locking was introduced for two reasons:
    • To make our remove fingerprint mode to work, without affecting other builds
    • To make targets atomic, in the sense that no other ongoing build could interfere with the copied artifacts.
  • Multiple targets were used to enable build parallelism (of different targets)

This PR removes the fingerprinting deletion code, (now --keep-ts will be required in COPY commands), and the responsibility of ensuring that the artifacts of concurrent builds do not overwrite each other is left to the user.

This results in a simpler API, and a reduced cache usage

@idelvall idelvall changed the title Remove fingerprints and locking Remove target locking Feb 6, 2024
@@ -1,96 +1,81 @@
VERSION --global-cache 0.7
VERSION --use-function-keyword --global-cache 0.7

Choose a reason for hiding this comment

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

--use-function-keyword only available in version 0.8 repo and samples pinned to version 0.7

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi Alex, that feature flag is available since 0.7.22 https://github.com/earthly/earthly/blob/v0.7.22/features/features.go#L67
Maybe your CLI is not updated?

Copy link
Collaborator

@xv-ian-c xv-ian-c left a comment

Choose a reason for hiding this comment

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

ensuring that the artifacts of concurrent builds do not overwrite each other is left to the user.

What does this mean for a user in practice?

cargo under the hood does lock things like the target dir, but is that enough for 90% of use cases? I'm not sure it is if you need to copy out the result.

Is DO rust+CARGO --args="build --release" --output="release/[^\./]+" guaranteed to run the build and the copy in some sort of atomic fashion avoiding any possible overwrite? If not how can a user arrange to avoid races using Earthly functionality?

There's another side effect of this change which is that, due to cargo locking the target directory, this will now effectively serialize all cargo-using jobs where previously they could run in parallel because they had their own target directory. That seems like a pretty crucial downside.

IMPORT github.com/earthly/lib/rust:<version/commit> AS rust
```
> :warning: Due to [this issue](https://github.com/earthly/earthly/issues/3490), make sure to enable `--global-cache` in the calling Earthfile, as shown above.
> *Due to [this issue](https://github.com/earthly/earthly/issues/3490), make sure to enable `--global-cache` in the calling Earthfile, as shown above.*
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is no longer shown above, probably because it's not needed with 0.8 any more?

@idelvall
Copy link
Member Author

I was assuming that Cargo locking was more fine gained (for target) than it apparently it is: rust-lang/cargo#2486.
Looks like target folder is locked globally so, we definitely don't want such limitation in Earthly builds.

@idelvall idelvall closed this Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants