Skip to content

solver: stub out sysSampler close#4775

Merged
tonistiigi merged 1 commit intomoby:masterfrom
jedevc:fix-sys-sampler-close
Mar 18, 2024
Merged

solver: stub out sysSampler close#4775
tonistiigi merged 1 commit intomoby:masterfrom
jedevc:fix-sys-sampler-close

Conversation

@jedevc
Copy link
Copy Markdown
Member

@jedevc jedevc commented Mar 18, 2024

Fixes #4774.

Follow-up to #4040 (cc @thaJeztah) - this didn't resolve the access to sysSampler.Close, we need to stub this one out as well!

Follow-up to ce332e1 - this didn't
resolve the access to sysSampler.Close, we need to stub this one out as
well!

Signed-off-by: Justin Chadwell <me@jedevc.com>
@thaJeztah
Copy link
Copy Markdown
Member

Changes SGTM, but also wondering; now that Windows is becoming an actual thing; should we consider newSysSampler() to return an actual sysSampler (with stubs for its methods)?

My slight concern here is that this will bring us down the road of other code hiding potential issues (if it's nil).

@jedevc
Copy link
Copy Markdown
Member Author

jedevc commented Mar 18, 2024

Changes SGTM, but also wondering; now that Windows is becoming an actual thing; should we consider newSysSampler() to return an actual sysSampler (with stubs for its methods)?

My slight concern here is that this will bring us down the road of other code hiding potential issues (if it's nil).

Sure, no reason to not do this IMO - we could have a dummySysSampler. That said, this is a super easy to cherry-pick change, and it's easy to manually verify that we've now got all the access points to this private field.

@thaJeztah
Copy link
Copy Markdown
Member

Yes, I guess for cherry-picking this one is fine to fix the immediate issue at hand.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM for the immediate issue (but not a maintainer)

@tonistiigi tonistiigi added this to the v0.13.1 milestone Mar 18, 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.

on windows, buildkitd.exe crashes when pressing Ctrl+C

3 participants