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

ci(deps): replace buildkit to fix fsutil issues on Windows #11426

Merged
merged 1 commit into from Jan 30, 2024

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Jan 30, 2024

What I did

Commit 7781b7c vendoring buildx master introduced a regression with a bump of the peer dependency github.com/tonistiigi/fsutil. full diff: tonistiigi/fsutil@36ef4d8...f098008

When bisecting, tonistiigi/fsutil#167 is the PR introducing the regression.

We got a similar issue reported before DD 4.27 (docker/buildx#2207) that was fixed with tonistiigi/fsutil@master...crazy-max:fsutil:toslash-keep-gogo but Windows users encountered another issue also related to fsutil.

As the deadline is closed for next DD 4.27 patch release and while a fix is being worked on fsutil repo to address this issue, I have created a branch that reverts this change in fsutil.

This branch for buildkit https://github.com/crazy-max/buildkit/tree/compose-957cb50df991 has been created at the regression point and reverts moby/buildkit#4094.

We are addressing this issue on fsutil with integration tests for Windows tonistiigi/fsutil#173 and are looking for a fix to this specific issue on buildx master.

Related issue

(not mandatory) A picture of a cute animal, if possible in relation to what you did

cc @glours @milas @thaJeztah

@crazy-max crazy-max marked this pull request as ready for review January 30, 2024 14:42
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ac8ea08) 56.63% compared to head (098878b) 56.59%.
Report is 1 commits behind head on main.

❗ Current head 098878b differs from pull request most recent head 7e913eb. Consider uploading reports for the commit 7e913eb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11426      +/-   ##
==========================================
- Coverage   56.63%   56.59%   -0.04%     
==========================================
  Files         136      136              
  Lines       11543    11543              
==========================================
- Hits         6537     6533       -4     
- Misses       4382     4386       +4     
  Partials      624      624              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

go.mod Outdated
Comment on lines 187 to 188
// reverts https://github.com/moby/buildkit/pull/4094 to fix fsutil issues on Windows
github.com/docker/buildx => github.com/crazy-max/buildx v0.8.1-0.20240130141015-d7042ae5516c // compose-617f538cb315
github.com/moby/buildkit => github.com/crazy-max/buildkit v0.7.1-0.20240130133234-d9aa289bd124 // compose-957cb50df991
Copy link
Member

Choose a reason for hiding this comment

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

Do we have PRs for this in the upstream repositories? Looks like compose is using master / v0.13.x as dependency, so if we could get the fix merged in upstream, that'd be good, I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both don't fix the actual issue but reverts moby/buildkit#4094. I hope to have it fix in fsutil so we can vendor in buildkit then buildx and compose to be aligned and remove these replaces 🙏.

Let me know if you prefer a branch on upstream repos instead.

// reverts https://github.com/moby/buildkit/pull/4094 to fix fsutil issues on Windows
github.com/docker/buildx => github.com/crazy-max/buildx v0.8.1-0.20240130141015-d7042ae5516c // compose-617f538cb315
github.com/moby/buildkit => github.com/crazy-max/buildkit v0.7.1-0.20240130133234-d9aa289bd124 // compose-957cb50df991
github.com/tonistiigi/fsutil => github.com/tonistiigi/fsutil v0.0.0-20230629203738-36ef4d8c0dbb
Copy link
Member

Choose a reason for hiding this comment

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

If this is "ahead" of the one at https://github.com/docker/compose/blob/058200181006be6565108d8b9ab12ce9b0101d6f/go.mod#L140C2-L140C30

Then we probably don't need a replace rule (but wee could add a comment after the // indirect to indicate we manually bumped (temporarily)).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this was mainly to group them under the same context to keep track

Commit 7781b7c vendoring buildx master introduced a regression
with a bump of the peer dependency github.com/tonistiigi/fsutil.
full diff: tonistiigi/fsutil@36ef4d8...f098008

When bisecting, tonistiigi/fsutil#167 is the PR introducing the
regression.

We got a similar issue reported before DD 4.27 (docker/buildx#2207)
that was fixed with tonistiigi/fsutil@master...crazy-max:fsutil:toslash-keep-gogo
but Windows users encountered another new issue also related to fsutil.

While a fix is being worked on fsutil repo to address this issue, I have
created a branch that reverts this change in fsutil.

This branch for buildkit https://github.com/crazy-max/buildkit/tree/compose-957cb50df991
has been created at the regression point and reverts moby/buildkit#4094.

Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
@crazy-max crazy-max changed the title replace buildx with custom fork to fix fsutil issues on Windows replace buildkit with custom fork to fix fsutil issues on Windows Jan 30, 2024
Copy link
Member

@milas milas left a comment

Choose a reason for hiding this comment

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

LGTM - tested latest revision on Win11 with a project that fails in Compose 2.24 and it builds properly now 👍🏻

@milas milas changed the title replace buildkit with custom fork to fix fsutil issues on Windows ci(deps): replace buildkit to fix fsutil issues on Windows Jan 30, 2024
@milas milas merged commit a0954dc into docker:main Jan 30, 2024
24 checks passed
@crazy-max crazy-max deleted the replace-buildx branch January 30, 2024 15:54
@crazy-max
Copy link
Member Author

Fyi, issue has been fixed in fsutil (tonistiigi/fsutil#187). Will be vendored on buildx repo for next v0.13.0.

rarguelloF pushed a commit to rarguelloF/compose that referenced this pull request Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants