code review 155160043: cmd/go: force rebuild of imported "main" packages

cmd/go: force rebuild of imported "main" packages

`go build` doesn't differentiate between building a "main" package into an
archive and building it into an executable.  As a result, if the executable
exists but the archive does not, it is considered non-stale, even if an
archive is needed by a dependent package.

This change causes us to always rebuild imported "main" packages as a
workaround until `go build` is made aware of the difference between building a
package into an archive and building a package into an executable.

Fixes issue 4210.

[kevinwallace, 2014-10-09 00:46:16]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-10-09 20:48:03]

R=rsc,adg


On Thu, Oct 9, 2014 at 2:46 AM, <kevin@pentabarf.net> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
>       cmd/go: force rebuild of imported "main" packages
>
> `go build` doesn't differentiate between building a "main" package into
> an
> archive and building it into an executable.  As a result, if the
> executable
> exists but the archive does not, it is considered non-stale, even if an
> archive is needed by a dependent package.
>
> This change causes us to always rebuild imported "main" packages as a
> workaround until `go build` is made aware of the difference between
> building a
> package into an archive and building a package into an executable.
>
> Fixes issue 4210.
>
> Please review this at https://codereview.appspot.com/155160043/
>
> Affected files (+7, -1 lines):
>   M src/cmd/go/pkg.go
>
>
> Index: src/cmd/go/pkg.go
> ===================================================================
> --- a/src/cmd/go/pkg.go
> +++ b/src/cmd/go/pkg.go
> @@ -570,6 +570,12 @@
>                         continue
>                 }
>                 p1 := loadImport(path, p.Dir, stk, p.build.ImportPos[path])
> +               if p1.Name == "main" {
> +                       // Force rebuilding of imported "main" packages,
> +                       // until we differentiate between building as an
> archive and as an executable.
> +                       // See https://code.google.com/p/go/
> issues/detail?id=4210
> +                       p1.forceBuild = true
> +               }
>                 if p1.local {
>                         if !p.local && p.Error == nil {
>                                 p.Error = &PackageError{
> @@ -723,7 +729,7 @@
>                 pkgBuildA = false
>         }
>
> -       if pkgBuildA || p.target == "" || p.Stale {
> +       if pkgBuildA || p.target == "" || p.Stale || p.forceBuild {
>                 return true
>         }
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[kevinwallace, 2014-10-28 18:58:43]

Ping?  Hope I'm not being overbearing, but I suspect this got lost in the noise
of dotGo.

On 2014/10/09 20:48:03, bradfitz wrote:
> R=rsc,adg
> 
> 
> On Thu, Oct 9, 2014 at 2:46 AM, <mailto:kevin@pentabarf.net> wrote:
> 
> > Reviewers: golang-codereviews,
> >
> > Message:
> > Hello mailto:golang-codereviews@googlegroups.com,
> >
> > I'd like you to review this change to
> > https://code.google.com/p/go
> >
> >
> > Description:
> >       cmd/go: force rebuild of imported "main" packages
> >
> > `go build` doesn't differentiate between building a "main" package into
> > an
> > archive and building it into an executable.  As a result, if the
> > executable
> > exists but the archive does not, it is considered non-stale, even if an
> > archive is needed by a dependent package.
> >
> > This change causes us to always rebuild imported "main" packages as a
> > workaround until `go build` is made aware of the difference between
> > building a
> > package into an archive and building a package into an executable.
> >
> > Fixes issue 4210.
> >
> > Please review this at https://codereview.appspot.com/155160043/
> >
> > Affected files (+7, -1 lines):
> >   M src/cmd/go/pkg.go
> >
> >
> > Index: src/cmd/go/pkg.go
> > ===================================================================
> > --- a/src/cmd/go/pkg.go
> > +++ b/src/cmd/go/pkg.go
> > @@ -570,6 +570,12 @@
> >                         continue
> >                 }
> >                 p1 := loadImport(path, p.Dir, stk, p.build.ImportPos[path])
> > +               if p1.Name == "main" {
> > +                       // Force rebuilding of imported "main" packages,
> > +                       // until we differentiate between building as an
> > archive and as an executable.
> > +                       // See https://code.google.com/p/go/
> > issues/detail?id=4210
> > +                       p1.forceBuild = true
> > +               }
> >                 if p1.local {
> >                         if !p.local && p.Error == nil {
> >                                 p.Error = &PackageError{
> > @@ -723,7 +729,7 @@
> >                 pkgBuildA = false
> >         }
> >
> > -       if pkgBuildA || p.target == "" || p.Stale {
> > +       if pkgBuildA || p.target == "" || p.Stale || p.forceBuild {
> >                 return true
> >         }
> >
> >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "golang-codereviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
> >

[adg1, 2014-10-28 23:35:05]

This is one for russ to review.

https://codereview.appspot.com/155160043/diff/20002/src/cmd/go/pkg.go
File src/cmd/go/pkg.go (right):

https://codereview.appspot.com/155160043/diff/20002/src/cmd/go/pkg.go#newcode576
src/cmd/go/pkg.go:576: // See https://code.google.com/p/go/issues/detail?id=4210
https://golang.org/issue/4210

[kevinwallace, 2014-10-29 23:58:04]

https://codereview.appspot.com/155160043/diff/20002/src/cmd/go/pkg.go
File src/cmd/go/pkg.go (right):

https://codereview.appspot.com/155160043/diff/20002/src/cmd/go/pkg.go#newcode576
src/cmd/go/pkg.go:576: // See https://code.google.com/p/go/issues/detail?id=4210
On 2014/10/28 23:35:05, adg1 wrote:
> https://golang.org/issue/4210

Done.

[rsc, 2014-10-30 00:49:00]

I don't understand. You are not supposed to be able to import packages
named main. Those are commands. The fact that this is even possible is the
real bug. I will comment on the bug. I won't touch this for Go 1.4 but for
Go 1.5 I think we should just make that an error.

[gobot, 2014-12-19 05:14:14]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/155160043 is best) in the description in your
new CL.

Thanks very much.