Merge pull request #869 from cpunion/fix-linking

Fix linking args, force doc test run on current ref

.github/workflows/doc.yml

@@ -24,9 +24,7 @@ jobs:
         run: npx embedme --verify README.md
 
   doc_test:
-    continue-on-error: true
     strategy:
-      fail-fast: false
       matrix:
         os:
           - macos-latest
@@ -58,6 +56,14 @@ jobs:
         run: |
           set -e
           set -x
+          git() {
+            if [ "$1" = "clone" ]; then
+              # do nothing because we already have the branch
+              cd ..
+            else
+              command git "$@"
+            fi
+          }
           source doc/_readme/scripts/install_llgo.sh
 
       - name: Test doc code blocks

internal/build/build.go

@@ -204,19 +204,16 @@ func Do(args []string, conf *Config) {
 	ctx := &context{env, cfg, progSSA, prog, dedup, patches, make(map[string]none), initial, mode, 0}
 	pkgs := buildAllPkgs(ctx, initial, verbose)
 
-	var llFiles []string
 	dpkg := buildAllPkgs(ctx, altPkgs[noRt:], verbose)
+	var linkArgs []string
 	for _, pkg := range dpkg {
-		if !strings.HasSuffix(pkg.ExportFile, ".ll") {
+		linkArgs = append(linkArgs, pkg.LinkArgs...)
-			continue
-		}
-		llFiles = append(llFiles, pkg.ExportFile)
 	}
 	if mode != ModeBuild {
 		nErr := 0
 		for _, pkg := range initial {
 			if pkg.Name == "main" {
-				nErr += linkMainPkg(ctx, pkg, pkgs, llFiles, conf, mode, verbose)
+				nErr += linkMainPkg(ctx, pkg, pkgs, linkArgs, conf, mode, verbose)
 			}
 		}
 		if nErr > 0 {
@@ -287,14 +284,14 @@ func buildAllPkgs(ctx *context, initial []*packages.Package, verbose bool) (pkgs
 			pkg.ExportFile = ""
 		case cl.PkgLinkIR, cl.PkgLinkExtern, cl.PkgPyModule:
 			if len(pkg.GoFiles) > 0 {
-				cgoParts, err := buildPkg(ctx, aPkg, verbose)
+				cgoLdflags, err := buildPkg(ctx, aPkg, verbose)
 				if err != nil {
 					panic(err)
 				}
 				linkParts := concatPkgLinkFiles(ctx, pkg, verbose)
-				allParts := append(linkParts, cgoParts...)
+				allParts := append(linkParts, cgoLdflags...)
 				allParts = append(allParts, pkg.ExportFile)
-				pkg.ExportFile = " " + strings.Join(allParts, " ")
+				aPkg.LinkArgs = allParts
 			} else {
 				// panic("todo")
 				// TODO(xsw): support packages out of llgo
@@ -304,66 +301,61 @@ func buildAllPkgs(ctx *context, initial []*packages.Package, verbose bool) (pkgs
 				// need to be linked with external library
 				// format: ';' separated alternative link methods. e.g.
 				//   link: $LLGO_LIB_PYTHON; $(pkg-config --libs python3-embed); -lpython3
-				expd := ""
 				altParts := strings.Split(param, ";")
+				expdArgs := make([]string, 0, len(altParts))
 				for _, param := range altParts {
 					param = strings.TrimSpace(param)
 					if strings.ContainsRune(param, '$') {
-						expd = env.ExpandEnv(param)
+						expdArgs = append(expdArgs, env.ExpandEnvToArgs(param)...)
 						ctx.nLibdir++
 					} else {
-						expd = param
+						expdArgs = append(expdArgs, param)
 					}
-					if len(expd) > 0 {
+					if len(expdArgs) > 0 {
 						break
 					}
 				}
-				if expd == "" {
+				if len(expdArgs) == 0 {
 					panic(fmt.Sprintf("'%s' cannot locate the external library", param))
 				}
 
-				command := ""
+				pkgLinkArgs := make([]string, 0, 3)
-				if expd[0] == '-' {
+				if expdArgs[0][0] == '-' {
-					command += " " + expd
+					pkgLinkArgs = append(pkgLinkArgs, expdArgs...)
 				} else {
-					linkFile := expd
+					linkFile := expdArgs[0]
 					dir, lib := filepath.Split(linkFile)
-					command = " -l " + lib
+					pkgLinkArgs = append(pkgLinkArgs, "-l"+lib)
 					if dir != "" {
-						command += " -L " + dir[:len(dir)-1]
+						pkgLinkArgs = append(pkgLinkArgs, "-L"+dir)
 						ctx.nLibdir++
 					}
 				}
-				if err := clangCheck.CheckLinkArgs(command); err != nil {
+				if err := clangCheck.CheckLinkArgs(pkgLinkArgs); err != nil {
-					panic(fmt.Sprintf("test link args '%s' failed\n\texpanded to: %s\n\tresolved to: %v\n\terror: %v", param, expd, command, err))
+					panic(fmt.Sprintf("test link args '%s' failed\n\texpanded to: %v\n\tresolved to: %v\n\terror: %v", param, expdArgs, pkgLinkArgs, err))
-				}
-				if isSingleLinkFile(pkg.ExportFile) {
-					pkg.ExportFile = command + " " + pkg.ExportFile
-				} else {
-					pkg.ExportFile = command + pkg.ExportFile
 				}
+				aPkg.LinkArgs = append(aPkg.LinkArgs, pkgLinkArgs...)
 			}
 		default:
-			cgoParts, err := buildPkg(ctx, aPkg, verbose)
+			cgoLdflags, err := buildPkg(ctx, aPkg, verbose)
 			if err != nil {
 				panic(err)
 			}
-			allParts := append(cgoParts, pkg.ExportFile)
+			aPkg.LinkArgs = append(cgoLdflags, pkg.ExportFile)
-			pkg.ExportFile = " " + strings.Join(allParts, " ")
 			setNeedRuntimeOrPyInit(pkg, prog.NeedRuntime, prog.NeedPyInit)
 		}
 	}
 	return
 }
 
-func linkMainPkg(ctx *context, pkg *packages.Package, pkgs []*aPackage, llFiles []string, conf *Config, mode Mode, verbose bool) (nErr int) {
+func linkMainPkg(ctx *context, pkg *packages.Package, pkgs []*aPackage, linkArgs []string, conf *Config, mode Mode, verbose bool) (nErr int) {
 	pkgPath := pkg.PkgPath
 	name := path.Base(pkgPath)
 	app := conf.OutFile
 	if app == "" {
 		app = filepath.Join(conf.BinPath, name+conf.AppExt)
 	}
-	args := make([]string, 0, len(pkg.Imports)+len(llFiles)+16)
+	args := make([]string, 0, len(pkg.Imports)+len(linkArgs)+16)
 	args = append(
 		args,
 		"-o", app,
@@ -396,9 +388,14 @@ func linkMainPkg(ctx *context, pkg *packages.Package, pkgs []*aPackage, llFiles
 	}
 	needRuntime := false
 	needPyInit := false
+	pkgsMap := make(map[*packages.Package]*aPackage, len(pkgs))
+	for _, v := range pkgs {
+		pkgsMap[v.Package] = v
+	}
 	packages.Visit([]*packages.Package{pkg}, nil, func(p *packages.Package) {
 		if p.ExportFile != "" { // skip packages that only contain declarations
-			args = appendLinkFiles(args, p.ExportFile)
+			aPkg := pkgsMap[p]
+			args = append(args, aPkg.LinkArgs...)
 			need1, need2 := isNeedRuntimeOrPyInit(p)
 			if !needRuntime {
 				needRuntime = need1
@@ -419,9 +416,7 @@ func linkMainPkg(ctx *context, pkg *packages.Package, pkgs []*aPackage, llFiles
 
 	dirty := false
 	if needRuntime {
-		for _, file := range llFiles {
+		args = append(args, linkArgs...)
-			args = appendLinkFiles(args, file)
-		}
 	} else {
 		dirty = true
 		fn := aPkg.LPkg.FuncOf(cl.RuntimeInit)
@@ -495,7 +490,7 @@ func linkMainPkg(ctx *context, pkg *packages.Package, pkgs []*aPackage, llFiles
 	return
 }
 
-func buildPkg(ctx *context, aPkg *aPackage, verbose bool) (cgoParts []string, err error) {
+func buildPkg(ctx *context, aPkg *aPackage, verbose bool) (cgoLdflags []string, err error) {
 	pkg := aPkg.Package
 	pkgPath := pkg.PkgPath
 	if debugBuild || verbose {
@@ -521,7 +516,7 @@ func buildPkg(ctx *context, aPkg *aPackage, verbose bool) (cgoParts []string, er
 		cl.SetDebug(0)
 	}
 	check(err)
-	cgoParts, err = buildCgo(ctx, aPkg, aPkg.Package.Syntax, externs, verbose)
+	cgoLdflags, err = buildCgo(ctx, aPkg, aPkg.Package.Syntax, externs, verbose)
 	if needLLFile(ctx.mode) {
 		pkg.ExportFile += ".ll"
 		os.WriteFile(pkg.ExportFile, []byte(ret.String()), 0644)
@@ -572,6 +567,8 @@ type aPackage struct {
 	SSA    *ssa.Package
 	AltPkg *packages.Cached
 	LPkg   llssa.Package
+
+	LinkArgs []string
 }
 
 func allPkgs(ctx *context, initial []*packages.Package, verbose bool) (all []*aPackage, errs []*packages.Package) {
@@ -590,7 +587,7 @@ func allPkgs(ctx *context, initial []*packages.Package, verbose bool) (all []*aP
 					return
 				}
 			}
-			all = append(all, &aPackage{p, ssaPkg, altPkg, nil})
+			all = append(all, &aPackage{p, ssaPkg, altPkg, nil, nil})
 		} else {
 			errs = append(errs, p)
 		}
@@ -692,18 +689,6 @@ func checkFlag(arg string, i *int, verbose *bool, swflags map[string]bool) {
 	}
 }
 
-func appendLinkFiles(args []string, file string) []string {
-	if isSingleLinkFile(file) {
-		return append(args, file)
-	}
-	// TODO(xsw): consider filename with spaces
-	return append(args, strings.Split(file[1:], " ")...)
-}
-
-func isSingleLinkFile(ret string) bool {
-	return len(ret) > 0 && ret[0] != ' '
-}
-
 func concatPkgLinkFiles(ctx *context, pkg *packages.Package, verbose bool) (parts []string) {
 	llgoPkgLinkFiles(ctx, pkg, func(linkFile string) {
 		parts = append(parts, linkFile)
@@ -729,9 +714,9 @@ func clFiles(ctx *context, files string, pkg *packages.Package, procFile func(li
 	args := make([]string, 0, 16)
 	if strings.HasPrefix(files, "$") { // has cflags
 		if pos := strings.IndexByte(files, ':'); pos > 0 {
-			cflags := env.ExpandEnv(files[:pos])
+			cflags := env.ExpandEnvToArgs(files[:pos])
 			files = files[pos+1:]
-			args = append(args, strings.Split(cflags, " ")...)
+			args = append(args, cflags...)
 		}
 	}
 	for _, file := range strings.Split(files, ";") {

internal/build/cgo.go

@@ -28,12 +28,13 @@ import (
 	"strings"
 
 	"github.com/goplus/llgo/internal/buildtags"
+	"github.com/goplus/llgo/internal/safesplit"
 )
 
 type cgoDecl struct {
 	tag     string
-	cflags  string
+	cflags  []string
-	ldflags string
+	ldflags []string
 }
 
 type cgoPreamble struct {
@@ -50,7 +51,7 @@ static void* _Cmalloc(size_t size) {
 `
 )
 
-func buildCgo(ctx *context, pkg *aPackage, files []*ast.File, externs map[string][]string, verbose bool) (cgoParts []string, err error) {
+func buildCgo(ctx *context, pkg *aPackage, files []*ast.File, externs map[string][]string, verbose bool) (cgoLdflags []string, err error) {
 	cfiles, preambles, cdecls, err := parseCgo_(pkg, files)
 	if err != nil {
 		return
@@ -66,11 +67,11 @@ func buildCgo(ctx *context, pkg *aPackage, files []*ast.File, externs map[string
 	ldflags := []string{}
 	for _, cdecl := range cdecls {
 		if cdecl.tag == "" || tagUsed[cdecl.tag] {
-			if cdecl.cflags != "" {
+			if len(cdecl.cflags) > 0 {
-				cflags = append(cflags, cdecl.cflags)
+				cflags = append(cflags, cdecl.cflags...)
 			}
-			if cdecl.ldflags != "" {
+			if len(cdecl.ldflags) > 0 {
-				ldflags = append(ldflags, cdecl.ldflags)
+				ldflags = append(ldflags, cdecl.ldflags...)
 			}
 		}
 	}
@@ -84,7 +85,7 @@ func buildCgo(ctx *context, pkg *aPackage, files []*ast.File, externs map[string
 	}
 	for _, cfile := range cfiles {
 		clFile(ctx, cflags, cfile, pkg.ExportFile, func(linkFile string) {
-			cgoParts = append(cgoParts, linkFile)
+			cgoLdflags = append(cgoLdflags, linkFile)
 		}, verbose)
 	}
 	re := regexp.MustCompile(`^(_cgo_[^_]+_Cfunc_)(.*)$`)
@@ -117,10 +118,12 @@ func buildCgo(ctx *context, pkg *aPackage, files []*ast.File, externs map[string
 			return nil, err
 		}
 		clFile(ctx, cflags, tmpName, pkg.ExportFile, func(linkFile string) {
-			cgoParts = append(cgoParts, linkFile)
+			cgoLdflags = append(cgoLdflags, linkFile)
 		}, verbose)
 	}
-	cgoParts = append(cgoParts, ldflags...)
+	for _, ldflag := range ldflags {
+		cgoLdflags = append(cgoLdflags, safesplit.SplitPkgConfigFlags(ldflag)...)
+	}
 	return
 }
 
@@ -294,18 +297,18 @@ func parseCgoDecl(line string) (cgoDecls []cgoDecl, err error) {
 		}
 		cgoDecls = append(cgoDecls, cgoDecl{
 			tag:     tag,
-			cflags:  strings.TrimSpace(string(cflags)),
+			cflags:  safesplit.SplitPkgConfigFlags(string(cflags)),
-			ldflags: strings.TrimSpace(string(ldflags)),
+			ldflags: safesplit.SplitPkgConfigFlags(string(ldflags)),
 		})
 	case "CFLAGS":
 		cgoDecls = append(cgoDecls, cgoDecl{
 			tag:    tag,
-			cflags: arg,
+			cflags: safesplit.SplitPkgConfigFlags(arg),
 		})
 	case "LDFLAGS":
 		cgoDecls = append(cgoDecls, cgoDecl{
 			tag:     tag,
-			ldflags: arg,
+			ldflags: safesplit.SplitPkgConfigFlags(arg),
 		})
 	}
 	return

internal/safesplit/safesplit.go

@@ -0,0 +1,86 @@
+/*
+ * Copyright (c) 2024 The GoPlus Authors (goplus.org). All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package safesplit
+
+import "strings"
+
+// SplitPkgConfigFlags splits a pkg-config outputs string into parts.
+// Each part starts with "-" followed by a single character flag.
+// Spaces after the flag character are ignored.
+// Content is read until the next space, unless escaped with "\".
+func SplitPkgConfigFlags(s string) []string {
+	var result []string
+	var current strings.Builder
+	i := 0
+
+	// Skip leading whitespace
+	for i < len(s) && (s[i] == ' ' || s[i] == '\t') {
+		i++
+	}
+
+	for i < len(s) {
+		// Start a new part
+		if current.Len() > 0 {
+			result = append(result, strings.TrimSpace(current.String()))
+			current.Reset()
+		}
+		// Write "-" and the flag character
+		current.WriteByte('-')
+		i++
+		if i < len(s) {
+			current.WriteByte(s[i])
+			i++
+		}
+		// Skip spaces after flag character
+		for i < len(s) && (s[i] == ' ' || s[i] == '\t') {
+			i++
+		}
+		// Read content until next space
+		for i < len(s) {
+			if s[i] == '\\' && i+1 < len(s) && (s[i+1] == ' ' || s[i+1] == '\t') {
+				// Skip backslash and write the escaped space
+				i++
+				current.WriteByte(s[i])
+				i++
+				continue
+			}
+			if s[i] == ' ' || s[i] == '\t' {
+				// Skip consecutive spaces
+				j := i
+				for j < len(s) && (s[j] == ' ' || s[j] == '\t') {
+					j++
+				}
+				// If we've seen content, check for new flag
+				if j < len(s) && s[j] == '-' {
+					i = j
+					break
+				}
+				// Otherwise, include one space and continue
+				current.WriteByte(' ')
+				i = j
+			} else {
+				current.WriteByte(s[i])
+				i++
+			}
+		}
+	}
+	// Add the last part
+	if current.Len() > 0 {
+		result = append(result, strings.TrimSpace(current.String()))
+	}
+	return result
+}

internal/safesplit/safesplit_test.go

@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2024 The GoPlus Authors (goplus.org). All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package safesplit
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestSplitPkgConfigFlags(t *testing.T) {
+	ftest := func(s, want string) {
+		t.Helper() // for better error message
+		got := toString(SplitPkgConfigFlags(s))
+		if got != want {
+			t.Errorf("\nSplitPkgConfigFlags(%q) =\n got %v\nwant %v", s, got, want)
+		}
+	}
+
+	t.Run("basic", func(t *testing.T) {
+		ftest("-I/usr/include -L/usr/lib", `["-I/usr/include" "-L/usr/lib"]`)
+		ftest("-I /usr/include -L /usr/lib", `["-I/usr/include" "-L/usr/lib"]`)
+		ftest("-L/opt/homebrew/Cellar/bdw-gc/8.2.8/lib -lgc",
+			`["-L/opt/homebrew/Cellar/bdw-gc/8.2.8/lib" "-lgc"]`)
+	})
+
+	t.Run("spaces_in_path", func(t *testing.T) {
+		ftest("-I/usr/local/include directory -L/usr/local/lib path",
+			`["-I/usr/local/include directory" "-L/usr/local/lib path"]`)
+	})
+
+	t.Run("multiple_spaces", func(t *testing.T) {
+		ftest("  -I  /usr/include   -L   /usr/lib  ", `["-I/usr/include" "-L/usr/lib"]`)
+	})
+
+	t.Run("consecutive_flags", func(t *testing.T) {
+		ftest("-I -L", `["-I-L"]`)
+		ftest("-I -L /usr/lib", `["-I-L /usr/lib"]`)
+	})
+
+	t.Run("edge_cases", func(t *testing.T) {
+		ftest("", "[]")
+		ftest("   ", "[]")
+		ftest("-", `["-"]`)
+		ftest("-I", `["-I"]`)
+		ftest("-I -", `["-I-"]`)
+	})
+
+	t.Run("escaped_spaces", func(t *testing.T) {
+		ftest(`-I/path\ with\ spaces -L/lib`, `["-I/path with spaces" "-L/lib"]`)
+		ftest(`-I /first\ path -L /second\ long path`, `["-I/first path" "-L/second long path"]`)
+	})
+
+	t.Run("macro_flags", func(t *testing.T) {
+		ftest("-DMACRO -I/usr/include", `["-DMACRO" "-I/usr/include"]`)
+		ftest("-D MACRO -I/usr/include", `["-DMACRO" "-I/usr/include"]`)
+		ftest("-DMACRO=value -I/usr/include", `["-DMACRO=value" "-I/usr/include"]`)
+		ftest("-D MACRO=value -I/usr/include", `["-DMACRO=value" "-I/usr/include"]`)
+		ftest("-D_DEBUG -D_UNICODE -DWIN32", `["-D_DEBUG" "-D_UNICODE" "-DWIN32"]`)
+		ftest("-D _DEBUG -D _UNICODE -D WIN32", `["-D_DEBUG" "-D_UNICODE" "-DWIN32"]`)
+		ftest("-DVERSION=2.1 -DDEBUG=1", `["-DVERSION=2.1" "-DDEBUG=1"]`)
+		ftest("-D VERSION=2.1 -D DEBUG=1", `["-DVERSION=2.1" "-DDEBUG=1"]`)
+	})
+}
+
+func toString(ss []string) string {
+	if ss == nil {
+		return "[]"
+	}
+	s := "["
+	for i, v := range ss {
+		if i > 0 {
+			s += " "
+		}
+		v = strings.ReplaceAll(v, `"`, `\"`)
+		s += `"` + v + `"`
+	}
+	return s + "]"
+}

xtool/clang/check/check.go

@@ -7,8 +7,7 @@ import (
 	"strings"
 )
 
-func CheckLinkArgs(args string) error {
+func CheckLinkArgs(cmdArgs []string) error {
-	cmdArgs := strings.Split(args, " ")
 	cmd := exec.Command("clang")
 	nul := "/dev/null"
 	if runtime.GOOS == "windows" {

xtool/env/env.go

@@ -22,6 +22,8 @@ import (
 	"os/exec"
 	"regexp"
 	"strings"
+
+	"github.com/goplus/llgo/internal/safesplit"
 )
 
 var (
@@ -29,6 +31,10 @@ var (
 	reFlag   = regexp.MustCompile(`[^ \t\n]+`)
 )
 
+func ExpandEnvToArgs(s string) []string {
+	return safesplit.SplitPkgConfigFlags(expandEnvWithCmd(s))
+}
+
 func ExpandEnv(s string) string {
 	return expandEnvWithCmd(s)
 }