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

goutil.Run(), goutil.Install() will potentially use outdated packages. #2

Closed
dmitshur opened this issue Jan 6, 2014 · 20 comments
Closed

Comments

@dmitshur
Copy link

dmitshur commented Jan 6, 2014

Since the generated .go file resides somewhere in a temporary folder, it is outside the GOPATH boundary, and if a package is built but outdated (i.e. source code has been changed since it was built), the outdated version will be used.

(From shurcooL/goexec#3.)

If I'm not mistaken, this issue affects you too, since you also use ioutil.TempDir with blank dir parameter for your temporary GOPATH workspace. So the folder will be somewhere in /tmp or similar, which will be outside the GOPATH boundary of imports.

Any thoughts on this?

@jimmyfrasche
Copy link
Owner

It also adds the tmp dir to the environment used when the go tool is
executed, but for some reason I create the src subdir in the tmp twice, so
thanks for making me look!

@dmitshur
Copy link
Author

dmitshur commented Jan 6, 2014

I noticed the double src, and wasn't sure if that was intentional or not. It's still valid, you're basically creating a package named "src" hehe.

Adding the tmp dir to GOPATH env var will not help prevent this issue, since the imported (potentially stale) package will likely be inside another GOPATH workspace (i.e. across the GOPATH workspace boundary, as Russ Cox phrased it). So I don't think this issue is fixed.

The go command stops at GOPATH workspace boundaries. If you tell it to build something in one workspace it will not build stale things in other workspaces.

@jimmyfrasche
Copy link
Owner

Oh interesting/annoying. I did not know that. I guess it'll have to use -a

@dmitshur
Copy link
Author

dmitshur commented Jan 6, 2014

Nice idea, I didn't think of using -a. Clearly, it's best if it doesn't have to be used since it will slow down compilation time. So it's a speed vs. "risk of potentially using stale imports" trade-off.

Personally, I don't see any good reasons why the go tool will use stale libraries if they happen to fall outside the GOPATH workspace your package is in. I thought having multiple GOPATH workspaces was simply an organizational aspect with no such side-effects. Similar to how you can take any Go package and split it into multiple .go files with no weird side-effects.

Perhaps we can make a case for it on that issue?

If you have one workspace per project you are not using them in the intended manner, which will make the tools work less well for you. See http://golang.org/doc/code.html for an overview.

What if you simply have 2 GOPATH workspaces? Then you're still affected. I don't get their logic here.


Another potential fix idea I had is to use to create the temporary folder within the user's GOPATH workspace, rather than inside a temporary OS folder. However, if the user has multiple GOPATH workspaces, there's still a chance of using stale libraries. :(

@dmitshur
Copy link
Author

dmitshur commented Jan 6, 2014

I'm currently looking at how the Go tool computes what packages are stale. Perhaps we can detect when there are stale imports, and then use -a flag. Otherwise, -a wouldn't be needed (and this is likely the most common scenario, but executing correctly 98% of the time isn't good enough...).

This may work, but... it feels like a hack and not the right way to do things, though. :/

@jimmyfrasche
Copy link
Owner

yeah, it would be nice if that worked as we and others thought it would,
but given the situation -a's the only option that works unconditionally at
the present time that I can see.

I'm trying to think of why go build would intentionally work as it does.
Maybe there's a good reason I can't see. I'd at least like an explanation.
Unless the reason is "stop using so many @#% workspaces" maybe we can do a
feature request for something between -a and the current behavior, like a
mode where it builds across workspaces . . .

@dmitshur
Copy link
Author

dmitshur commented Jan 6, 2014

I'm trying to think of why go build would intentionally work as it does.
Maybe there's a good reason I can't see. I'd at least like an explanation.

Yeah, same here.

I think we should point out the potential problem with the current approach and inquire about their motivation behind this decision. (Unless I'm missing something,) things would be so much simpler if the go tool always tried to build stale libraries, whether they're in a different GOPATH workspace or not.

@jimmyfrasche
Copy link
Owner

Ugh, I already duplicated build tag parsing and copied the code for
parsing docstrings.

If I'm going to go through all that trouble I might as well, find out
which packages are stale and just build them beforehand: that'd
definitely be better than -a. Of course, neither really fits in the
interface of gostrap. I suppose I could have put a Stale method on
Package . . .

As long as I'm doing all this I might as well move gostrap into its
own package since it's the odd man out in this one . . .

As for getting an explanation, maybe just a post on the mailing list?
Any requests on that bug report would at most be tangential and it
might be easier for future seekers to find if it's its own thread
somewhere. If you want I can do that, just let me know. Otherwise fire
away.

@dmitshur
Copy link
Author

dmitshur commented Jan 6, 2014

As for getting an explanation, maybe just a post on the mailing list?

I've found some explanation in the go source code, where it determines if a package is stale or not:

// isStale reports whether package p needs to be rebuilt.
func isStale(p *Package, topRoot map[string]bool) bool {
...
    // Have installed copy, probably built using current compilers,
    // and built after its imported packages.  The only reason now
    // that we'd have to rebuild it is if the sources were newer than
    // the package.   If a package p is not in the same tree as any
    // package named on the command-line, assume it is up-to-date
    // no matter what the modification times on the source files indicate.
    // This avoids rebuilding $GOROOT packages when people are
    // working outside the Go root, and it effectively makes each tree
    // listed in $GOPATH a separate compilation world.
    // See issue 3149.
    if p.Root != "" && !topRoot[p.Root] {
        return false
    }
...

It links to issue 3149. Reading that stuff now.

@dmitshur
Copy link
Author

dmitshur commented Jan 6, 2014

Issue 3149 makes it seem that the reason they did this "don't touch other GOPATH workspaces" was from the era of Go when setting the GOROOT envvar was required, and clearly you don't want go build something to be trying to compile/installing anything in GOROOT. So maybe this is no longer necessary now that GOROOT is treated somewhat separately from GOPATH workspaces? I think this issue is definitely worth revisiting and further consideration.

My proposal would be to only build across other GOPATH workspaces, but never inside GOROOT. I think that'd be quite awesome (and it seems like a very easy fix, no hard implementation work required here).

Any requests on that bug report would at most be tangential and it
might be easier for future seekers to find if it's its own thread
somewhere. If you want I can do that, just let me know. Otherwise fire
away.

If you could do that, I'd prefer and appreciate that... I'm feeling tired today lol.

@dmitshur
Copy link
Author

For reference, the go-nuts thread is here (thanks for making it btw!):

https://groups.google.com/forum/#!topic/golang-nuts/Z5uJf6mEF6E

Sadly, it's not getting much attention so far (and got hard to find, lol).

Btw, since this GH issue is still closed, are you not considering it an issue?

@jimmyfrasche
Copy link
Owner

I forgot I closed it because I was replying to the e-mail notifications. I'm so bad at management things. Thanks for the reminder/bump.

I am considering a semi-issue. It's a problem but without a change to the go tool, there's not much to do except throw a lot of code at it and document where it behaves differently.

I'd like to fix it but for now the duct tape that is -a will have to do.

@jimmyfrasche jimmyfrasche reopened this Jan 26, 2014
@dmitshur
Copy link
Author

Some progress on the issue. I got the following response from minux on golang-nuts:

On Mon, Jan 27, 2014 at 9:14 PM, Dmitri Shuralyov shur...@gmail.com wrote:
While you're evaluating this, could you please take a second look at https://code.google.com/p/go/source/browse/src/cmd/go/pkg.go#676 ? It is somewhat relevant to the legacy of GOROOT and GOPATH, and IMO an optimal decision today may differ from the optimal back when that decision was made.

See this thread for more details: https://groups.google.com/forum/#!topic/golang-nuts/Z5uJf6mEF6E

Yes, I've seen that discussion and the issue filed on the tracker.

As Russ is away for the moment, who is responsible for the Go command, I suggest you
raise the question to golang-dev (open a new thread) to get feedback from other developers.

@dmitshur
Copy link
Author

I suggest you raise the question to golang-dev (open a new thread) to get feedback from other developers.

I think we should do that. I'll try to do that in the next few days.

@dmitshur
Copy link
Author

I've reposted your (excellent) original post in golang-dev here.

@dmitshur
Copy link
Author

dmitshur commented Jun 4, 2015

Woohoo!!! golang/go#10509 has been resolved. Once 1.5 comes out, both this and shurcooL/goexec#3 can be resolved.

@jimmyfrasche
Copy link
Owner

huzzah!

@dmitshur
Copy link
Author

dmitshur commented Nov 1, 2015

Hey @jimmyfrasche,

Now that Go 1.5 is out, you can resolve this issue very easily. It's just a matter of removing the -a flag that we added as a temporary workaround.

See shurcooL/goexec#5 for an example of how I resolved it in goe.

@jimmyfrasche
Copy link
Owner

Thanks for the reminder. Finally closed!

@dmitshur
Copy link
Author

dmitshur commented Nov 7, 2015

Congratulations! 🎉 🎊 ✨

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

No branches or pull requests

2 participants