-
Notifications
You must be signed in to change notification settings - Fork 23
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
goversion: rewrite on top of cmd/go/internal/version #17
base: master
Are you sure you want to change the base?
Conversation
Imported the cmd/go/internal/version implementation at c32140fa94cfc51a2152855825f57e27ae3ba133, and restored Symbols and TextRange to implement the gccgo and crypto checks. It looks like the amd64 matching had broken, but it should be more stable to just track what we do upstream with minimal modifications, so I replaced the core mechanism rather than fixing the matcher. Fixes rsc#14 Fixes rsc#12 Fixes rsc#11 Fixes rsc#7 Closes rsc#13 Closes rsc#9
Pulling in the tests from #14, this approach seems to work for go1.13+, but fails for binaries built with older go versions:
on master, those tests fail on go1.14+ but work for binaries built with older go versions
|
that seems more likely to make the identification work for binaries built with the newest go version, but lose the ability to recognize binaries built with older versions |
I think the idea has potential though... would it make sense to just leave multiple implementations in their own packages and try them in turn until one succeeds?
|
I see no reason whatsoever to delete the old functionality. It works for older versions and those versions are not changing. If you revise the PR to simply add the new checkers without dropping the old ones, I'm happy to merge that. |
@FiloSottile are you planning to make the requested changes to the PR? Would be great to see those changes in. |
Imported the cmd/go/internal/version implementation at
golang/go@c32140f, and restored Symbols and
TextRange to implement the gccgo and crypto checks.
It looks like the amd64 matching had broken, but it should be more
stable to just track what we do upstream with minimal modifications, so
I replaced the core mechanism rather than fixing the matcher.
Fixes #14
Fixes #12
Fixes #11
Fixes #7
Closes #13
Closes #9