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: provide package path for main packages to cmd/compile #34480

Open
mdempsky opened this issue Sep 23, 2019 · 4 comments
Open

cmd/go: provide package path for main packages to cmd/compile #34480

mdempsky opened this issue Sep 23, 2019 · 4 comments
Labels
FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@mdempsky
Copy link
Member

Can cmd/go provide cmd/compile with the full package path to the source package, even when compiling main packages?

When benchmarking cmd/compile changes, it's useful to key stuff by myimportpath (i.e., the -p command-line flag) and just spit everything across an entire "go build -a std cmd" build into a single file, and then let benchcmp or benchstat handle it.

But this currently doesn't work for main packages, because cmd/go sets -p main for these:

pkgpath := p.ImportPath
if cfg.BuildBuildmode == "plugin" {
pkgpath = pluginPath(a)
} else if p.Name == "main" && !p.Internal.ForceLibrary {
pkgpath = "main"
}
gcargs := []string{"-p", pkgpath}

So you end up with a bunch of "BenchmarkFoo:main" lines, which muddle the benchcmp/benchstat output.

I figure two main options:

  1. Change cmd/go to just set -p to the package path regardless, and cmd/compile can rewrite it to "main" where/if necessary. (Looking briefly at myimportpath, some uses would be unaffected; but DWARF and the new ABI stuff might be impacted.)

  2. Add another command-line flag for cmd/go to provide the package path, which cmd/compile can use for tagging benchmarking data with instead.

/cc @rsc @aclements

@mdempsky mdempsky added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. FeatureRequest labels Sep 23, 2019
@jayconrod
Copy link
Contributor

cc @cherrymui

@cherrymui
Copy link
Member

The compiler uses myimportpath for symbol names and such. But as you said we can rewrite it to "main" as necessary. So either option will work for this use case.

I'm slightly inclined to option 2, for an option specific for benchmarking. User could also overwrite it if necessary.

@jayconrod
Copy link
Contributor

For option 1, a binary may have multiple packages named main. That comes up a lot when testing a main package and when using -coverpkg with a pattern that covers multiple main packages. So don't rewrite in any context where this needs to be unique.

For option 2, would you want to use this for import paths (strings that appear in import declarations)? Import paths are user-friendly but may not be unique. Package paths are unique but less friendly (resolved vendor directories and such). Currently, when a package is compiled, the compiler does not know how it will be imported by other packages. A package may be imported with multiple distinct import paths because of minimal module compatibility, too, so we have to be careful here.

@mdempsky
Copy link
Member Author

I'm slightly inclined to option 2, for an option specific for benchmarking. User could also overwrite it if necessary.

That makes sense.

So don't rewrite in any context where this needs to be unique.

Perhaps cmd/go could provide -p "foo/bar" -main instead of rewriting -p "foo/bar" into -p "main"? That would let cmd/compile know when it should use "main" instead.

For option 2, would you want to use this for import paths (strings that appear in import declarations)? Import paths are user-friendly but may not be unique. Package paths are unique but less friendly (resolved vendor directories and such).

Hm, so I want that when I run "go build [pkgs...]", that each individual compile operation has some unique stable identifier so I can correlate benchmarks across builds.

That makes me think we actually want the resolved package path, which then also means we can't always rely on -p even for non-main packages.

@seankhliao seankhliao added this to the Unplanned milestone Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants