Issue 42910043: code review 42910043: cmd/go: avoid use of 'go tool pack' (Closed)
Descriptioncmd/go: avoid use of 'go tool pack'
All packages now use the -pack option to the compiler.
For a pure Go package, that's enough.
For a package with additional C and assembly files, the extra
archive entries can be added directly (by concatenation)
instead of by invoking go tool pack.
These changes make it possible to rewrite cmd/pack in Go.
Patch Set 1 #Patch Set 2 : diff -r 4dd891cd0a26 https://code.google.com/p/go/ #
Total comments: 10
Patch Set 3 : diff -r e8c8b0e2a52f https://code.google.com/p/go/ #MessagesTotal messages: 17
Hello iant, r (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
LGTM https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1608: if internalPack && cmd == "rqP" { the cmd== is a bit of a hack. worth a comment at least.
Sign in to reply to this message.
https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1593: const internalPack = true Do you want this to be submitted? https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1615: if err := packInternal(b, mkAbs(objDir, afile), absOfiles); err != nil { Use absAfile here. https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1629: dst, err := os.OpenFile(afile, os.O_WRONLY, 0) Could use O_APPEND here and skip the Seek. https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1633: defer dst.Close() It always makes me uncomfortable to see a Close of a file opened for writing that does not check for an error. We want this to be reliable so I think it should check for a Close error one way or another.
Sign in to reply to this message.
https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1593: const internalPack = true On 2013/12/17 01:09:36, iant wrote: > Do you want this to be submitted? I was planning to, but removed. https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1608: if internalPack && cmd == "rqP" { On 2013/12/16 19:14:12, r wrote: > the cmd== is a bit of a hack. worth a comment at least. Done (new bool variable instead). https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1615: if err := packInternal(b, mkAbs(objDir, afile), absOfiles); err != nil { On 2013/12/17 01:09:36, iant wrote: > Use absAfile here. Done. https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1629: dst, err := os.OpenFile(afile, os.O_WRONLY, 0) On 2013/12/17 01:09:36, iant wrote: > Could use O_APPEND here and skip the Seek. Done. https://codereview.appspot.com/42910043/diff/20001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1633: defer dst.Close() On 2013/12/17 01:09:36, iant wrote: > It always makes me uncomfortable to see a Close of a file opened for writing > that does not check for an error. We want this to be reliable so I think it > should check for a Close error one way or another. Done.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=4bdb5c0bd2be *** cmd/go: avoid use of 'go tool pack' All packages now use the -pack option to the compiler. For a pure Go package, that's enough. For a package with additional C and assembly files, the extra archive entries can be added directly (by concatenation) instead of by invoking go tool pack. These changes make it possible to rewrite cmd/pack in Go. R=iant, r CC=golang-dev https://codereview.appspot.com/42910043
Sign in to reply to this message.
Message was sent while issue was closed.
It seems this change broke the build on the Plan 9 builder, but running ./all.rc manually still works fine. # Building packages and commands for plan9/386. runtime errors sync/atomic sync # sync pkg/sync/cond.go:8: can't find import: "sync/atomic" http://build.golang.org/log/e7c3fecaec378cd81fbb6a4a6126a7ec4a5eea52
Sign in to reply to this message.
Interesting. That would probably be the first package that contains ASM but not c code. > On 18 Dec 2013, at 20:53, 0intro@gmail.com wrote: > > It seems this change broke the build on the Plan 9 builder, > but running ./all.rc manually still works fine. > > # Building packages and commands for plan9/386. > runtime > errors > sync/atomic > sync > # sync > pkg/sync/cond.go:8: can't find import: "sync/atomic" > > http://build.golang.org/log/e7c3fecaec378cd81fbb6a4a6126a7ec4a5eea52 > > > https://codereview.appspot.com/42910043/ > > -- > > ---You received this message because you are subscribed to the Google Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out.
Sign in to reply to this message.
Message was sent while issue was closed.
I've tracked down the issue. The builder ends with files named "/tmp/gobuilder/plan9-386-cnielsen-b77dd2c5702d/go/pkg/plan9_386/math.a" instead of "math.a" in /tmp/go-build885854060. So it seems to be an issue with os.Rename.
Sign in to reply to this message.
On Wed, Dec 18, 2013 at 3:20 PM, <0intro@gmail.com> wrote: > I've tracked down the issue. > > The builder ends with files named > "/tmp/gobuilder/plan9-386-cnielsen-b77dd2c5702d/go/pkg/plan9_386/math.a" > instead of "math.a" in /tmp/go-build885854060. So it seems to be an > issue with os.Rename. > Yes, os.Rename should be rejecting cross-directory renames and is not. But are you saying that in /tmp/go-buildxxx there is a single file with slashes in its name? Probably the kernel and the file server should have rejected the wstat too, and neither did. Russ
Sign in to reply to this message.
Message was sent while issue was closed.
> But are you saying that in /tmp/go-buildxxx there is a single file with slashes in its name? No, this the case of every files, including the sub-directories. cpu% cd /tmp/go-build631669448 ; ls /tmp/gobuilder/plan9-386-cnielsen-c448cd5b51d9/go/pkg/plan9_386/encoding.a /tmp/gobuilder/plan9-386-cnielsen-c448cd5b51d9/go/pkg/plan9_386/errors.a /tmp/gobuilder/plan9-386-cnielsen-c448cd5b51d9/go/pkg/plan9_386/math.a /tmp/gobuilder/plan9-386-cnielsen-c448cd5b51d9/go/pkg/plan9_386/runtime.a /tmp/gobuilder/plan9-386-cnielsen-c448cd5b51d9/go/pkg/plan9_386/sort.a /tmp/gobuilder/plan9-386-cnielsen-c448cd5b51d9/go/pkg/plan9_386/unicode.a container crypto image log runtime sync unicode cpu% cd container ; ls /tmp/gobuilder/plan9-386-cnielsen-c448cd5b51d9/go/pkg/plan9_386/container/list.a /tmp/gobuilder/plan9-386-cnielsen-c448cd5b51d9/go/pkg/plan9_386/container/ring.a
Sign in to reply to this message.
Sorry, I phrased the question badly. I was really just asking if the file name itself had slashes, and the answer is yes. Congratulations, you've found three bugs: one in Go, one in the Plan 9 kernel, and one in the file server being used for /tmp. Russ
Sign in to reply to this message.
Oh, and I apologize for the trouble you are going to have removing those files. Russ
Sign in to reply to this message.
Message was sent while issue was closed.
I don't think the problem is related to os.Rename. It seems to correctly return an error when renaming a file to another directory. One thing I don't understand yet is why it only happens with the builder, and not when running ./all.rc manually.
Sign in to reply to this message.
Message was sent while issue was closed.
I'm now able to reproduce the problem. The function os.Rename doesn't return a proper error when dst is longer than 63 bytes.
Sign in to reply to this message.
Message was sent while issue was closed.
I propose CL 43480050 to work around this issue. Fossil should probably be fixed too. Don't worry about the broken file names, I remove them with fscons clri.
Sign in to reply to this message.
Thanks. You should also fix os.rename. If newname has slashes, the directory part must be lexically the same as the directory part of oldname, and then use only the final element in the wstat. Otherwise, the wstat should not even be attempted. Russ
Sign in to reply to this message.
|
