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

add GetGoRoot #42

Merged
merged 13 commits into from
Jul 8, 2021
Merged

add GetGoRoot #42

merged 13 commits into from
Jul 8, 2021

Conversation

boy-hack
Copy link
Contributor

No description provided.

Copy link
Member

@TcM1911 TcM1911 left a comment

Choose a reason for hiding this comment

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

Thank you for contributing. I think this is a great additional feature to the project.

I've provided some suggestions and before I can merge this I'd like to have some tests around this. The project uses a set of known golden files that can be used for tests. There are some helper functions to resolve the paths for the files. See TestGetTypes for how they can be used. The CONTRIBUTING.md doc explains how you can either download prebuilt binaries or build them yourself.

All the golden files should have a GOROOT of /usr/local/go.

goroot.go Show resolved Hide resolved
goroot.go Outdated Show resolved Hide resolved
goroot.go Outdated Show resolved Hide resolved
goroot.go Outdated Show resolved Hide resolved
goroot.go Outdated Show resolved Hide resolved
goroot.go Show resolved Hide resolved
goroot.go Outdated Show resolved Hide resolved
goroot.go Show resolved Hide resolved
@boy-hack
Copy link
Contributor Author

boy-hack commented Jul 1, 2021

Hello, I have solved it according to the suggestion and added a test case to goroot_test.go, but the test case shows that the go version from 1.5 to 1.9 will not be recognized. I don’t know how to modify this part

@TcM1911 TcM1911 added the enhancement New feature or request label Jul 2, 2021
@TcM1911
Copy link
Member

TcM1911 commented Jul 2, 2021

Can you take a look at how the string is addressed in the older versions? Strings are represented as:

type str struct {
  data ptr
  len int
}

In the logic for extracting the version string, it looks for a pointer to the beginning of this struct. After that I read either two uint32 or uint64 depending on word size used to get were the string starts and the length. It's possible that in older versions, the address and length are directly moved into the register without dereference it. That may be the issue.

@TcM1911 TcM1911 added this to the v0.9.0 milestone Jul 2, 2021
@boy-hack
Copy link
Contributor Author

boy-hack commented Jul 5, 2021

I have added the code to get goroot to support go versions between 1.5 and 1.9. It supports darwin and linux. Windows doesn’t seem to work because I didn’t find goroot string in the windows binary.

@boy-hack boy-hack requested a review from TcM1911 July 6, 2021 10:14
@TcM1911
Copy link
Member

TcM1911 commented Jul 6, 2021

It looks like the functionality you are using in the time package was added in Go 1.10. For the Windows test cases 1.5 to 1.9, can you add a check that the correct error is returned instead?

@boy-hack
Copy link
Contributor Author

boy-hack commented Jul 6, 2021

@TcM1911 I don't know how to make elegant test cases, can you help me?

@TcM1911
Copy link
Member

TcM1911 commented Jul 6, 2021

The variable test should be the filename, so you could do something like this after the Open call.

switch test {
   case "gold-windows-amd64-1.9.0":
   case "gold-windows-386-1.9.0":
   ....
      r.Equal(Err.., err)
   default:
     // The original part of the code
 }

Just add a small comment to note why the specific files are checked for an error...

@boy-hack
Copy link
Contributor Author

boy-hack commented Jul 7, 2021

Thank you, I have updated it and now it works very well

@TcM1911 TcM1911 merged commit 147d337 into goretk:develop Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants