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

cmd/go: does not rebuild when source files are removed #3895

Closed
rogpeppe opened this issue Aug 2, 2012 · 34 comments
Closed

cmd/go: does not rebuild when source files are removed #3895

rogpeppe opened this issue Aug 2, 2012 · 34 comments
Labels
FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@rogpeppe
Copy link
Contributor

rogpeppe commented Aug 2, 2012

This is how I broke our build.

% pwd
/home/rog/src/go/src/local/gobug
% ls
% cat > foo.go
package foo
var X = 0
^D
% cat > bar.go
package foo
var Y = 0
^D
% cat > foo_test.go
package foo_test
import (
    "testing"
    foo "local/gobug"
)
func TestX(t *testing.T){
    _ = foo.X
}
^D
% go test
PASS
ok      local/gobug 0.011s
% go install
% rm foo.go      # as it happens, i actually moved it elsewhere.
% go build
% go install
% go test
PASS
ok      local/gobug 0.011s
# everything looks just fine, so i pushed to trunk here, breaking
# the build.
% touch *.go
% go test
# local/gobug_test
./foo_test.go:7: undefined: foo.X
FAIL    local/gobug [build failed]
% 

I wonder if the go tool should look at the mtime of the
directory as well as the source files when determining
whether to rebuild.
@robpike
Copy link
Contributor

robpike commented Aug 2, 2012

Comment 1:

Labels changed: added priority-soon, removed priority-triage.

Owner changed to @rsc.

@robpike
Copy link
Contributor

robpike commented Aug 2, 2012

Comment 2:

Status changed to Accepted.

@rogpeppe
Copy link
Contributor Author

Comment 3:

This issue also affects build when the build tags change.

@rsc
Copy link
Contributor

rsc commented Aug 31, 2012

Comment 4:

I am sorry you broke your build. However, I see no way to fix this.

@rsc
Copy link
Contributor

rsc commented Sep 12, 2012

Comment 5:

Labels changed: added go1.1.

@rsc
Copy link
Contributor

rsc commented Dec 10, 2012

Comment 6:

Labels changed: added size-l.

@rsc
Copy link
Contributor

rsc commented Jan 31, 2013

Comment 7:

Labels changed: added priority-later, removed priority-soon.

@rsc
Copy link
Contributor

rsc commented Mar 12, 2013

Comment 8:

Labels changed: added go1.1maybe, removed go1.1.

@robpike
Copy link
Contributor

robpike commented May 18, 2013

Comment 9:

Labels changed: added go1.2maybe, removed go1.1maybe.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 10:

Labels changed: added feature.

@robpike
Copy link
Contributor

robpike commented Aug 29, 2013

Comment 11:

Marking "will not fix" because I don't see how we can, practically.  Reopen if you can
suggest a solution. In any case, it won't be fixed in 1.2.

Labels changed: removed go1.2maybe.

Status changed to WontFix.

@bradfitz
Copy link
Contributor

Comment 12:

We can't check the mtime of the directory?  It will be updated when a file is deleted,
no?

@rsc
Copy link
Contributor

rsc commented Sep 5, 2013

Comment 13:

There are at least two possible fixes. One is to include in the .a file a hash of all
the source file content that went into it (or the file names and mtimes). The hash will
not match if a file is missing. Another, partial fix is to use a background process that
watches file changes for the go command. It would see the deletion and be able to tell
the go command that the package is out of date. That would speed ordinary builds too,
and is something I want to explore for 1.3.

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Sep 5, 2013

Comment 14:

Status changed to Accepted.

@rsc
Copy link
Contributor

rsc commented Sep 5, 2013

Comment 15:

Issue #6294 has been merged into this issue.

@bradfitz
Copy link
Contributor

bradfitz commented Sep 5, 2013

Comment 16:

Russ, you listed two possible fixes, but didn't comment on checking the mtime of the
directory.  What's the problem with that approach?

@rsc
Copy link
Contributor

rsc commented Sep 5, 2013

Comment 17:

That's possible too, although it might result in many false positives.

@gopherbot
Copy link

Comment 18:

#3832 leads me to thinking that hash should include build tags.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 19:

Labels changed: removed feature.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 20:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 21:

Labels changed: added repo-main.

@ianlancetaylor
Copy link
Contributor

Comment 22:

Labels changed: added suggested, removed priority-later.

@thockin
Copy link

thockin commented Aug 22, 2014

Comment 23:

I hit this this week, or something like it.  If the go tool can't track deps properly,
are they really that useful?
Is there a way to do builds that doesn't suffer from this?  Like, can I fall back on
plain old Makefiles and dep tracking?

@minux
Copy link
Member

minux commented Aug 22, 2014

Comment 24:

it's difficult to track this automatically without somehow
embedding all source files into the package archive.

@thockin
Copy link

thockin commented Aug 22, 2014

Comment 25:

I totally get that it is difficult, but it is BROKEN as it stands today, and there is no
particularly good workaround.
When "make" is a better build system that what you have created, you have a problem.

@rsc
Copy link
Contributor

rsc commented Aug 22, 2014

Comment 26:

Tim, I don't understand why you think make is a better build system. Make and all other
timestamp-based build systems have the same problem.

@thockin
Copy link

thockin commented Aug 23, 2014

Comment 27:

If I remove a file in a properly built Makefile, it fails to build.  It does a) what is
correct and b) what people expect. It comes at the cost of some extra metadata reads,
but at least it is correct.
I'm not trying to be antagonist, here, I'm just a bit incredulous that anyone defends
NOT fixing this.  Is there some perspective from which this behavior is "correct"?
thockin@thockin-glaptop:/tmp$ rm -f foo a b
thockin@thockin-glaptop:/tmp$ cat Makefile
all: foo
foo: a b
    cat a b > $@
a: a.in
    cat $^ | tr 'a-z' 'A-Z' > $@
b: b.in
    cat $^ | tr 'a-z' 'A-Z' > $@
thockin@thockin-glaptop:/tmp$ echo hello > a.in
thockin@thockin-glaptop:/tmp$ echo world > b.in
thockin@thockin-glaptop:/tmp$ make
cat a.in | tr 'a-z' 'A-Z' > a
cat b.in | tr 'a-z' 'A-Z' > b
cat a b > foo
thockin@thockin-glaptop:/tmp$ cat foo
HELLO
WORLD
thockin@thockin-glaptop:/tmp$ rm b.in
thockin@thockin-glaptop:/tmp$ make
make: *** No rule to make target `b.in', needed by `b'.  Stop.

@bradfitz
Copy link
Contributor

Comment 28:

Tim, nobody is arguing that this bug shouldn't be fixed.
In your Makefile example, the dependencies are all listed in the Makefile.
If you'd like to work on this, the comments in this bug suggest ways to fix it.

@thockin
Copy link

thockin commented Aug 23, 2014

Comment 29:

Typical to OSS, of course I don't have time to learn a new codebase and fix a bug deemed
"difficult" by the core team.  That's not fair.  This will probably make me go away,
which is what such a statement is really intended to do, but I go away muttering about
how broken go is, and how we all need to work around it.
Given that go says (more or less) "everyone should use our build tools" (and in fact
makes it more or less impossible NOT to), and those tools do not work correctly, you can
perhaps see why I am frustrated by the response?
My intention was not to stir up a fight, just to vocally +1 an obvious bug and make my
plea to not close it just because it is hard to fix.

@ianlancetaylor
Copy link
Contributor

Comment 30:

Don't worry, we aren't going to close it.  It clearly must be fixed.

@rsc
Copy link
Contributor

rsc commented Aug 23, 2014

Comment 31:

Tim, the issue is that the go command effectively creates the makefile on demand. The
analogous situation in make is if you had written the Makefile in a more general way
like:
ALLOUT=$(shell ls *.in | sed 's/\.in$$//')
all: foo
foo: $(ALLOUT)
    cat $(ALLOUT) > $@
%: %.in
    cat $^ | tr 'a-z' 'A-Z' > $@
In that case:
g% make foo
cat a.in | tr 'a-z' 'A-Z' > a
cat b.in | tr 'a-z' 'A-Z' > b
cat a b > foo
g% cat foo
HELLO
WORLD
g% rm b.in b
g% make foo
make: `foo' is up to date.
g% cat foo
HELLO
WORLD
g% make -B foo
cat a.in | tr 'a-z' 'A-Z' > a
cat a > foo
g% cat foo
HELLO
g% 
The reason things work in your version is that there is a duplication of information, so
that when you remove b and forget to update the makefile, there is an inconsistency that
stops the build, and get told things are wrong. But if you had also updated the makefile
(or been using a self-updating makefile like the one above) you'd run into the same
thing.
As I mentioned in #13, we had intended to fix this in most cases by watching for file
system updates, but that turned out to be very difficult to do in anything like a
portable way. Every OS has invented a different API with wildly different semantics for
that.
It looks like we are going to have to fall back to some kind of content-based hashing
instead of mtimes. But that's a big change, and we have higher priority bugs to fix
right now.
No one is saying it's not a problem, and no one is saying that we're just going to close
the bug.

@anacrolix
Copy link
Contributor

I take it in the example in comment 31, that 'a' and 'b' are analogous to object files, 'a.in' and 'b.in', source files, and that 'foo' is a package/library file, such as foo.a. If 'a.in' is newer than 'a', than 'a' is rebuilt, and then so is 'foo'. But why can't we have that if an object file exists, and a corresponding source file does not, then the extraneous object file is removed and the package rebuilt?

@tv42
Copy link

tv42 commented Jan 29, 2015

@anacrolix Source files and object files don't map 1:1, multiple sources are combined into one .a. See comments 13 and 16.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@gopherbot
Copy link

CL https://golang.org/cl/9154 mentions this issue.

@rsc rsc closed this as completed in 7b87631 Jun 4, 2015
@mikioh mikioh modified the milestones: Go1.5, Unplanned Jun 4, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
@rsc rsc removed their assignment Jun 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

No branches or pull requests