changelog entry, panic less often, reject newer Go versions, document init order caveat #835
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Panicking in small helpers or in funcs that don't return error has proved useful to keep code easier to maintain, particularly for cases that should typically never happen. However, in these cases we can error just as easily. In particular, I was getting a panic whenever I forgot that I was running garble with Go master (1.23), which is over the top.
Right now, we only have linker patches for Go 1.22.x.
We only ever maintain those for one or two major Go versions at a time.
If a user tries to use the Go toolchain from 1.21, we already fail
with "Go version too old" messages early on, but we don't for 1.23,
causing a relatively confusing error later on when we link a binary:
cannot get modified linker: cannot retrieve linker patches: open patches/go1.23: file does not exist
Instead, fail early and with a good error message.
Keeping the original lexical sorting of Go packages would be very hard, as a Go program may import an unknown number of Go packages, and we load and obfuscate one package at a time by design. One option would be to load all packages upfront when obfuscating main packages, but that would break the per-package caching of ofbuscated Go packages, causing a huge slow-down in builds. Another option would be to not obfuscate import paths, which would clearly cause a worsening of the obfuscation quality. The third option is to not attempt to keep the original order, and document that as a caveat in the README. I suspect the vast majority of Go projects won't be affected by this, and those few that might be can always use imports to enforce the order. Closes burrowers#693, per the decision above to not change what we do.
It accomplishes the same Implicits/Defs logic we were doing here.
|
The changelog entry was written in the release branch, so this adds it to master as well, for consistency. |
pagran
approved these changes
Feb 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(see commit messages - please do not squash)