From f5dcde646e7617035c286519f2a972373b4b7557 Mon Sep 17 00:00:00 2001 From: Jon Poole Date: Tue, 20 Jun 2023 15:35:44 +0100 Subject: [PATCH 1/2] WIP --- build_defs/cgo.build_defs | 1 + build_defs/go.build_defs | 5 +- tools/please_go/generate/generate.go | 156 ++++++++++++++++++++++++++- 3 files changed, 158 insertions(+), 4 deletions(-) diff --git a/build_defs/cgo.build_defs b/build_defs/cgo.build_defs index f583da15..b7c348f4 100644 --- a/build_defs/cgo.build_defs +++ b/build_defs/cgo.build_defs @@ -86,6 +86,7 @@ def cgo_library(name:str, srcs:list=[], resources:list=None, go_srcs:list=[], c_ 'cd "$TMP_DIR"', f'ls {subdir2}*.c {subdir2}*.go', ]), + deps = deps, tools = [CONFIG.GO.GO_TOOL], post_build = post_build if file_srcs != srcs else None, requires = ['go', 'cc_hdrs'], diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index ed94afdd..28df6bff 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -1160,18 +1160,21 @@ def go_repo(module: str, version:str=None, download:str=None, name:str=None, ins else: modFileArg = "" + src_root = f"pkg/{CONFIG.OS}_{CONFIG.ARCH}/{module}" + requirements = " ".join(requirements) repo = build_rule( name = name, tag = "repo" if install else None, srcs = srcs, - cmd = f"rm -rf $SRCS_DOWNLOAD/.plzconfig && find $SRCS_DOWNLOAD -name BUILD -delete && $TOOL generate {modFileArg} --module {module} --src_root=$SRCS_DOWNLOAD {install_args} {requirements} && mv $SRCS_DOWNLOAD $OUT", + cmd = f"rm -rf $SRCS_DOWNLOAD/.plzconfig && find $SRCS_DOWNLOAD -name BUILD -delete && mkdir -p $SRC_ROOT && mv $SRCS_DOWNLOAD/* $SRC_ROOT && $TOOL generate {modFileArg} --module {module} --src_root=$SRC_ROOT {install_args} {requirements} && mv $SRC_ROOT $OUT", outs = [subrepo_name], tools = [CONFIG.GO.PLEASE_GO_TOOL], env= { "GOOS": CONFIG.OS, "GOARCH": CONFIG.ARCH, "CGO_ENABLED": CONFIG.GO.CGO_ENABLED, + "SRC_ROOT": src_root, }, deps = [CONFIG.GO.MOD_FILE] if CONFIG.GO.MOD_FILE else None, _subrepo = True, diff --git a/tools/please_go/generate/generate.go b/tools/please_go/generate/generate.go index 3cd2a049..19625135 100644 --- a/tools/please_go/generate/generate.go +++ b/tools/please_go/generate/generate.go @@ -25,9 +25,18 @@ type Generate struct { knownImportTargets map[string]string // cache these so we don't end up looping over all the modules for every import thirdPartyFolder string install []string + // Any paths included via -I or -L in this module that we should generate filegroups for + localIncludePaths map[string]struct{} } func New(srcRoot, thirdPartyFolder, modFile, module string, buildFileNames, moduleDeps, install []string) *Generate { + // Ensure the srcRoot is absolute otherwise build.Import behaves badly + wd, err := os.Getwd() + if err != nil { + panic(err) + } + srcRoot = filepath.Join(wd, srcRoot) + return &Generate{ srcRoot: srcRoot, buildContext: build.Default, @@ -38,6 +47,7 @@ func New(srcRoot, thirdPartyFolder, modFile, module string, buildFileNames, modu thirdPartyFolder: thirdPartyFolder, install: install, moduleName: module, + localIncludePaths: map[string]struct{}{}, } } @@ -53,9 +63,61 @@ func (g *Generate) Generate() error { if err := g.generateAll(g.srcRoot); err != nil { return err } + if err := g.generateIncludeFilegroups(); err != nil { + return err + } return g.writeInstallFilegroup() } +func (g *Generate) generateIncludeFilegroups() error { + for pkg := range g.localIncludePaths { + if err := g.generateIncludeFilegroup(pkg); err != nil { + return err + } + } + return nil +} + +func (g *Generate) generateIncludeFilegroup(pkg string) error { + var srcs []string + var exportedDeps []string + + entries, err := os.ReadDir(filepath.Join(g.srcRoot, pkg)) + if err != nil { + return err + } + + for _, entry := range entries { + if !entry.IsDir() { + if g.isBuildFile(entry.Name()) { + continue + } + srcs = append(srcs, entry.Name()) + continue + } + + if err := g.generateIncludeFilegroup(filepath.Join(pkg, entry.Name())); err != nil { + return err + } + + exportedDeps = append(exportedDeps, buildTarget("inc", filepath.Join(pkg, entry.Name()), "")) + } + + file, err := parseOrCreateBuildFile(filepath.Join(g.srcRoot, pkg), g.buildFileNames) + if err != nil { + return err + } + + rule := NewRule("filegroup", "inc") + rule.SetAttr("srcs", NewStringList(srcs)) + rule.SetAttr("exported_deps", NewStringList(exportedDeps)) + rule.SetAttr("visibility", NewStringList([]string{"PUBLIC"})) + + file.Stmt = append(file.Stmt, rule.Call) + + return saveBuildFile(file) +} + func (g *Generate) installTargets() []string { var targets []string @@ -177,8 +239,41 @@ func (g *Generate) pkgDir(target string) string { } func (g *Generate) importDir(target string) (*build.Package, error) { - dir := filepath.Join(os.Getenv("TMP_DIR"), g.pkgDir(target)) - return g.buildContext.ImportDir(dir, 0) + i := "./" + trimPath(target, g.moduleName) + if i == "./." { + i = "." + } + pkg, err := g.buildContext.Import("./"+i, g.srcRoot, 0) + if err != nil { + return nil, err + } + + // We import with an absolute source dir which means some -I and -L args will include the current tmp dir. We should + // replace those with an env var that will be expanded correctly at build time. + replaceTmpDirWithEnvVar(pkg.CgoCFLAGS) + replaceTmpDirWithEnvVar(pkg.CgoCPPFLAGS) + replaceTmpDirWithEnvVar(pkg.CgoCXXFLAGS) + replaceTmpDirWithEnvVar(pkg.CgoLDFLAGS) + + return pkg, nil +} + +// trimPath is like strings.TrimPrefix but is path aware. It removes base from target if target starts with base, +// otherwise returns target unmodified. +func trimPath(target, base string) string { + baseParts := strings.Split(filepath.Clean(base), "/") + targetParts := strings.Split(filepath.Clean(target), "/") + + if len(targetParts) < len(baseParts) { + return target + } + + for i := range baseParts { + if baseParts[i] != targetParts[i] { + return target + } + } + return strings.Join(targetParts[len(baseParts):], "/") } func (g *Generate) generate(dir string) error { @@ -195,6 +290,21 @@ func (g *Generate) generate(dir string) error { return g.createBuildFile(dir, lib) } +func (g *Generate) getIncludeDepsFromCFlags(pkg *build.Package) []string { + goRootPath := `"$TMP_DIR"/pkg/` + g.buildContext.GOOS + "_" + g.buildContext.GOARCH + "/" + + var ret []string + for _, flag := range append(pkg.CgoCFLAGS, pkg.CgoLDFLAGS...) { + if strings.HasPrefix(flag, "-I"+goRootPath) { + ret = append(ret, g.includeTarget(strings.TrimPrefix(flag, "-I"+goRootPath))) + } + if strings.HasPrefix(flag, "-L"+goRootPath) { + ret = append(ret, g.includeTarget(strings.TrimPrefix(flag, "-L"+goRootPath))) + } + } + return ret +} + func (g *Generate) matchesInstall(dir string) bool { for _, i := range g.install { i := filepath.Join(g.srcRoot, i) @@ -341,11 +451,24 @@ func (g *Generate) libRule(pkg *build.Package) *Rule { pkgConfigs: pkg.CgoPkgConfig, asmFiles: pkg.SFiles, hdrs: pkg.HFiles, - deps: g.depTargets(pkg.Imports), + deps: append(g.getIncludeDepsFromCFlags(pkg), g.depTargets(pkg.Imports)...), embedPatterns: pkg.EmbedPatterns, } } +// replaceTmpDirWithEnvVar replaces the current wd with the env var $TMP_DIR. This helps in cases where ${SRC_DIR} has +// been resolved to the absolute path based on the current build directory. To make this portable, it needs to use the +// env var which will be expanded at build time. +func replaceTmpDirWithEnvVar(flags []string) { + tmpDir, err := os.Getwd() + if err != nil { + panic(err) + } + for i := range flags { + flags[i] = strings.ReplaceAll(flags[i], tmpDir, `"$TMP_DIR"`) + } +} + func (g *Generate) testRule(pkg *build.Package, prodRule *Rule) *Rule { // The name of the target should match the dir it's in, or the basename of the module if it's in the repo root. name := filepath.Base(pkg.Dir) @@ -371,6 +494,33 @@ func (g *Generate) testRule(pkg *build.Package, prodRule *Rule) *Rule { } } +func (g *Generate) includeTarget(path string) string { + module := "" + for _, mod := range append(g.moduleDeps, g.moduleName) { + if strings.HasPrefix(path, mod) { + if len(module) < len(mod) { + module = mod + } + } + } + + if module == "" { + // If we can't find this import, we can return nothing and the build rule will fail at build time reporting a + // sensible error. It may also be an import from the go SDK which is fine. + return "" + } + + subrepoName := g.subrepoName(module) + packageName := strings.TrimPrefix(path, module) + packageName = strings.TrimPrefix(packageName, "/") + + target := buildTarget("inc", packageName, subrepoName) + if module == g.moduleName { + g.localIncludePaths[packageName] = struct{}{} + } + return target +} + func (g *Generate) depTarget(importPath string) string { if target, ok := g.knownImportTargets[importPath]; ok { return target From 836cbd43e886bb69e6acfcb240dbcf6a99c7f9cf Mon Sep 17 00:00:00 2001 From: Jon Poole Date: Tue, 20 Jun 2023 18:12:50 +0100 Subject: [PATCH 2/2] Set LD flags on CGO rule like go build odes --- build_defs/cgo.build_defs | 40 ++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/build_defs/cgo.build_defs b/build_defs/cgo.build_defs index b7c348f4..4bca2469 100644 --- a/build_defs/cgo.build_defs +++ b/build_defs/cgo.build_defs @@ -74,18 +74,7 @@ def cgo_library(name:str, srcs:list=[], resources:list=None, go_srcs:list=[], c_ 'c': [subdir2 + src.replace('.go', '.cgo2.c') for src in file_srcs] + [subdir2 + '_cgo_export.c'], 'h': [subdir2 + '_cgo_export.h'], }, - cmd = ' && '.join([ - (f'OUT_DIR="$TMP_DIR/{subdir}"') if subdir else 'OUT_DIR="$TMP_DIR"', - 'mkdir -p "$OUT_DIR"', - 'cd $PKG_DIR/' + subdir, - f'$TOOL tool cgo -objdir "$OUT_DIR" -importpath {import_path} -- {compiler_flags_cmd} *.go', - # Remove the .o file which BSD sed gets upset about in the next command - 'rm -f "$OUT_DIR"/_cgo_.o "$OUT_DIR"/_cgo_main.c', - # cgo leaves absolute paths in these files which we must get rid of :( - 'find "$OUT_DIR" -type f -maxdepth 1 | xargs sed -i -e "s|"$TMP_DIR"/||g"', - 'cd "$TMP_DIR"', - f'ls {subdir2}*.c {subdir2}*.go', - ]), + cmd = _cgo_cmd(subdir, subdir2, import_path, compiler_flags_cmd, " ".join([f"'{flag}'" for flag in linker_flags])), deps = deps, tools = [CONFIG.GO.GO_TOOL], post_build = post_build if file_srcs != srcs else None, @@ -100,7 +89,7 @@ def cgo_library(name:str, srcs:list=[], resources:list=None, go_srcs:list=[], c_ compiler_flags = compiler_flags + [ '-Wno-error', '-Wno-unused-parameter', # Generated code doesn't compile clean - ], + ] + linker_flags, pkg_config_libs = pkg_config, test_only = test_only, deps = deps, @@ -144,3 +133,28 @@ def cgo_library(name:str, srcs:list=[], resources:list=None, go_srcs:list=[], c_ deps = deps, exported_deps=[import_config], ) + +def _cgo_cmd(subdir, subdir2, import_path, compiler_flags_cmd, linker_flags=None): + linker_flags = linker_flags or "" + return ' && '.join([ + f'export CGO_LDFLAGS="{linker_flags}"', + f'OUT_DIR="$TMP_DIR/{subdir}"' if subdir else 'OUT_DIR="$TMP_DIR"', + 'mkdir -p "$OUT_DIR"', + 'cd $PKG_DIR/' + subdir, + f'$TOOL tool cgo -objdir "$OUT_DIR" -importpath {import_path} -- {compiler_flags_cmd} *.go', + # Remove the .o file which BSD sed gets upset about in the next command + 'rm -f "$OUT_DIR"/_cgo_.o "$OUT_DIR"/_cgo_main.c', + # cgo leaves absolute paths in these files which we must get rid of :( + 'find "$OUT_DIR" -maxdepth 1 -type f | xargs sed -i -e "s|"$TMP_DIR"/||g"', + 'cd "$TMP_DIR"', + f'ls {subdir2}*.c {subdir2}*.go', + ]) + +def _collect_cgo_linker_flags(subdir, subdir2, import_path, compiler_flags_cmd): + """Returns a pre-build function to apply transitive linker flags to a go_binary rule.""" + def collect_linker_flags(name): + ldflags, _ = _get_ldflags_and_pkgconfig(name) + if ldflags: + set_command(name, _cgo_cmd(subdir, subdir2, import_path, compiler_flags_cmd)) + return collect_linker_flags +