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

pkg/reexec: cleanup and remove some dependencies #47932

Merged
merged 5 commits into from
Jun 10, 2024

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jun 8, 2024

pkg/reexec: don't mix syscall and golang.org/x/sys package

commit 069fdc8 changed most uses of
the syscall package to switch utsname from unsigned to signed (see
069fdc8). Those don't seem to be
impacting the code used here, so either stdlib or golang.org/x/sys/unix
should work for this case.

I chose stdlib's syscall package for this case, in case we'd decide to
move this package to a separate module (and want to limit its dependencies).

pkg/reexec: unify implementation of Self() and remove stub

This combines the implementations of the Self function, to allow having
a single GoDoc to document the behavior.

There is a minor change in behavior, as this patch removes the stub for
unsupported platforms (non-linux, windows, freebsd or darwin), which will
now use os.Args[0]. The stub was added in 21537b8
to fix compilation of https://github.com/ethereum/go-ethereum on OpenBSD,
which had docker/docker as dependency. It looks like that repository no
longer has this dependency, and as this was only to make the code
compilable, is unlikely to be a problem.

pkg/reexec: unify non-Linux implementation of Command

The Windows, Darwin, and FreeBSD implementations were identical, other
than their GoDoc to be different. Unify them so that we don't have to
maintain separate GoDoc for each.

It's worth noting that FreeBSD also supports Pdeathsig, so could be
using the same implementation as Linux. However, we don't test/maintain
the FreeBSD implementation, and it would require updating to GoDoc to
be more specific about the use of /proc/self/exe, so keeping the
status quo for now.

pkg/reexec: remove gotest.tools from tests

This package is used in BuildKit, and could be a potential candidate
for moving to a separate module. While it's not too problematic to have
this dependency, the tests only used basic assertions from gotest.tools,
which could be easily re-implemented without the dependency.

pkg/reexec: touch-up GoDoc, and remove "import" comments

Touch-up some GoDoc in the package, and remove "import" comments.

This package is used in BuildKit, and could be a potential candidate
for moving to a separate module. The "import" comments are ignored when
used in go module mode so have little benefit. Let's remove them.

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah added 5 commits June 8, 2024 14:18
commit 069fdc8 changed most uses of
the syscall package to switch utsname from unsigned to signed (see
069fdc8). Those don't seem to be
impacting the code used here, so either stdlib or golang.org/x/sys/unix
should work for this case.

I chose stdlib's syscall package for this case, in case we'd decide to
move this package to a separate module (and want to limit its dependencies).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This combines the implementations of the Self function, to allow having
a single GoDoc to document the behavior. The naiveSelf function is kept,
because it's used in unit-tests.

There is a minor change in behavior, as this patch removes the stub for
unsupported platforms (non-linux, windows, freebsd or darwin), which will
now use `os.Args[0]`. The stub was added in 21537b8
to fix compilation of https://github.com/ethereum/go-ethereum on OpenBSD,
which had docker/docker as dependency. It looks like that repository no
longer has this dependency, and as this was only to make the code
compilable, is unlikely to be a problem.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The Windows, Darwin, and FreeBSD implementations were identical, other
than their GoDoc to be different. Unify them so that we don't have to
maintain separate GoDoc for each.

It's worth noting that FreeBSD also supports Pdeathsig, so could be
using the same implementation as Linux. However, we don't test/maintain
the FreeBSD implementation, and it would require updating to GoDoc to
be more specific about the use of `/proc/self/exe`, so keeping the
status quo for now.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This package is used in BuildKit, and could be a potential candidate
for moving to a separate module. While it's not too problematic to have
this dependency, the tests only used basic assertions from gotest.tools,
which could be easily re-implemented without the dependency.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Touch-up some GoDoc in the package, and remove "import" comments.

This package is used in BuildKit, and could be a potential candidate
for moving to a separate module. The "import" comments are ignored when
used in go module mode so have little benefit. Let's remove them.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Jun 8, 2024
@thaJeztah thaJeztah self-assigned this Jun 8, 2024
@thaJeztah thaJeztah added this to the 27.0.0 milestone Jun 10, 2024
@thaJeztah thaJeztah merged commit 6e514e8 into moby:master Jun 10, 2024
135 checks passed
@thaJeztah thaJeztah deleted the reexec_clean branch June 10, 2024 09:31
@thaJeztah thaJeztah mentioned this pull request Oct 15, 2024
74 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor PR's that refactor, or clean-up code status/2-code-review
Projects
Development

Successfully merging this pull request may close these issues.

2 participants