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 boulder invocation via symlinks #7905

Merged
merged 4 commits into from
Dec 19, 2024
Merged

Conversation

mcpherrinm
Copy link
Contributor

@mcpherrinm mcpherrinm commented Dec 19, 2024

Boulder switched from multiple binaries to one by having symlinks for the old binaries, but we invoke boulder via subcommands now. This drops support for running via symlinks in Boulder, and drops them from the build process.

This does explicitly list out the four binaries in the makefile, which I think explicitly listing them is fine given that we rarely add them. This also avoids needing to duplicate mentioning the special ct-test-srv in the deb/tar rules. We could probably just look at what's in bin/ after go install ./..., but I didn't want to get too into makefile changes.

We haven't used the symlinked versions of commands for a while, and can drop them from builds.

This also drops the .rpm builds, which we also haven't used in a long time.

This explicitly lists out the four binaries we include in releases, and drops
the symlinks and link.sh.

I think explicitly listing them is fine given that we rarely add them. This
also avoids needing to duplicate mentioning the special ct-test-srv in the
deb/tar rules.

We haven't used the symlinked versions of commands for a while, and can drop
them from builds.
We haven't used this in a long time either.
@mcpherrinm mcpherrinm changed the title Remove symlinks from builds Remove boulder invocation via symlinks Dec 19, 2024
@mcpherrinm
Copy link
Contributor Author

the cmd/boulder/main.go changes are easier to read with "ignore whitespace" turned on, as I un-indented a section of code.

@mcpherrinm mcpherrinm marked this pull request as ready for review December 19, 2024 02:36
@mcpherrinm mcpherrinm requested a review from a team as a code owner December 19, 2024 02:36
@mcpherrinm mcpherrinm requested a review from jprenken December 19, 2024 02:36
@mcpherrinm mcpherrinm changed the title Remove boulder invocation via symlinks IN-10907: Remove boulder invocation via symlinks Dec 19, 2024
@mcpherrinm mcpherrinm changed the title IN-10907: Remove boulder invocation via symlinks Remove boulder invocation via symlinks Dec 19, 2024
@jprenken jprenken requested review from a team and aarongable and removed request for a team December 19, 2024 02:45
CMDS = $(shell find ./cmd -maxdepth 1 -mindepth 1 -type d | grep -v testdata)
CMD_BASENAMES = $(shell echo $(CMDS) | xargs -n1 basename)
CMD_BINS = $(addprefix bin/, $(CMD_BASENAMES) )
CMDS = admin boulder ceremony ct-test-srv
Copy link
Contributor

Choose a reason for hiding this comment

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

I have two very vague concerns about explicit listing these four binaries here:

  1. This is right at the top of the file, but ends up only being used by a few targets. In particular, "make build_cmds" will compile everything in the repo, including all the other test services, not just these four.
  2. It seems like it would be easy to create a new non-boulder executable and forget to add it to this list. Maybe that's fine -- this is intended to be the "what gets packaged" list -- but that circles back to my first point: this looks like the "what gets built" list.

I don't know that there's anything super actionable in this feedback. Feel free to ignore it -- I think there's actually a larger cleanup to be done here that I can tackle after this merges (why do we define OBJECTS to be a copy of CMD_BINS? why do we disallow saying "make boulder" or "make admin"? why do we allow saying "make bin/boulder", but when you do, it builds all commands not just that one?).

Copy link
Contributor Author

@mcpherrinm mcpherrinm Dec 19, 2024

Choose a reason for hiding this comment

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

I definitely don't love listing them here, but I decided against trying to do a larger makefile refactor as that's some makefile-writing neurons not fully activated right now. I'm not sure all the workflows used here, so didn't want to tinker too much.

@aarongable aarongable merged commit 1797450 into main Dec 19, 2024
12 checks passed
@aarongable aarongable deleted the mattm-new-year-new-you branch December 19, 2024 20:11
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.

3 participants