Skip to content

RFC: Remove drivers/windows #2284

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

Merged
merged 1 commit into from
Jul 18, 2025
Merged

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Mar 14, 2025

Compare #2009 : The code could never build its unit tests since September 2017, and nobody seems to have complained.

Also, see how this impacts go.mod: many of the largest (google.golang.org/grpc), obsolete (github.com/gogo/protobuf) or outright frozen and unmaintained (go.opencensus.io), or just aesthetically unappealing (github.com/containerd/…) dependencies go away.

[And then we could discuss the unmaintained github.com/json-iterator/go, a codebase full of reflect and unsafe. That’s also fun.]

To be fair, containers/ocicrypt drags GRPC into Podman anyway, and several dependencies add various implementations of protobuf.

I’m not going to strongly advocate merging this, but I did want to show that, look, this is clearly not in a good shape, and the cost is not all that small, just as one more data point for the conversation (so, marking as draft, ~indefinitely).

Cc: @giuseppe @nalind @mheon

But then again, would this impact plans for containers/buildah#4010 ? Do we instead need to start seriously caring? Cc: @sebsoto

@giuseppe
Copy link
Member

Cc: @giuseppe @nalind @mheon

But then again, would this impact plans for containers/buildah#4010 ? Do we instead need to start seriously caring? Cc: @sebsoto

I am in favor of simplifying code that is out of maintenance (as it appears to be in this case). (+1. -89,831) is a significant improvement, even if it doesn't affect the binary size

@mheon
Copy link
Member

mheon commented Mar 14, 2025

LGTM on my end

@Luap99
Copy link
Member

Luap99 commented Mar 14, 2025

But then again, would this impact plans for containers/buildah#4010 ? Do we instead need to start seriously caring?

Properly best to wait on some come of outcome on that discussion to see whenever that will be a priority or not. Removing this now just to add it back in a few months doesn't seem particular helpful either.

The diff looks very appealing though, so I would love for this to just be gone...

The code could never build its unit tests since September 2017,
and nobody seems to have complained.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 17, 2025

Four months later…

I want to acknowledge that some discussions about native Windows support are ongoing, but so far I don’t read them to suggest that it is very likely to happen. (Also, none of the proponents of Windows support have spoken to support this in this repo, AFAIK) Please correct me if anything of the above is wrong.

So, shall we pull the plug and delete the driver now? We can always revert later. Marking as “Ready for review”.

@mtrmac mtrmac marked this pull request as ready for review July 17, 2025 17:36
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

openshift-ci bot commented Jul 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, Luap99, mtrmac

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit f555b9f into containers:main Jul 18, 2025
20 checks passed
@mtrmac mtrmac deleted the no-windows branch July 18, 2025 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants