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

Improve comment stripping errors #1675

Merged
merged 1 commit into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/md/melange_compile.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,13 @@ melange compile [flags]
--env-file string file to use for preloaded environment variables
--fail-on-lint-warning turns linter warnings into failures
--generate-index whether to generate APKINDEX.tar.gz (default true)
--git-commit string commit hash of the git repository containing the build config file (defaults to detecting HEAD)
--git-repo-url string URL of the git repository containing the build config file (defaults to detecting from configured git remotes)
--guest-dir string directory used for the build environment guest
-h, --help help for compile
-i, --interactive when enabled, attaches stdin with a tty to the pod on failure
-k, --keyring-append strings path to extra keys to include in the build environment keyring
--license string license to use for the build config file itself (default "NOASSERTION")
--log-policy strings logging policy to use (default [builtin:stderr])
--memory string default memory resources to use for builds
--namespace string namespace to use in package URLs in SBOM (eg wolfi, alpine) (default "unknown")
Expand Down
9 changes: 5 additions & 4 deletions pkg/build/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,6 @@ func New(ctx context.Context, opts ...Option) (*Build, error) {
if b.ConfigFileRepositoryCommit == "" {
return nil, fmt.Errorf("config file repository commit was not set")
}
if b.Runner == nil {
return nil, fmt.Errorf("no runner was specified")
}

parsedCfg, err := config.ParseConfiguration(ctx,
b.ConfigFile,
Expand Down Expand Up @@ -717,6 +714,10 @@ func (b *Build) BuildPackage(ctx context.Context) error {
ctx, span := otel.Tracer("melange").Start(ctx, "BuildPackage")
defer span.End()

if b.Runner == nil {
return fmt.Errorf("no runner was specified")
}

b.summarize(ctx)

namespace := b.Namespace
Expand Down Expand Up @@ -779,7 +780,7 @@ func (b *Build) BuildPackage(ctx context.Context) error {

log.Infof("evaluating pipelines for package requirements")
if err := b.Compile(ctx); err != nil {
return fmt.Errorf("compiling build: %w", err)
return fmt.Errorf("compiling %s: %w", b.ConfigFile, err)
}

// Filter out any subpackages with false If conditions.
Expand Down
33 changes: 27 additions & 6 deletions pkg/build/compile.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (t *Test) Compile(ctx context.Context) error {

// We want to evaluate this but not accumulate its deps.
if err := ignore.CompilePipelines(ctx, sm, cfg.Pipeline); err != nil {
return fmt.Errorf("compiling main pipelines: %w", err)
return fmt.Errorf("compiling package %q pipelines: %w", t.Package, err)
}

for i, sp := range cfg.Subpackages {
Expand Down Expand Up @@ -103,7 +103,7 @@ func (t *Test) Compile(ctx context.Context) error {
}

if err := test.CompilePipelines(ctx, sm, cfg.Test.Pipeline); err != nil {
return fmt.Errorf("compiling main test pipelines: %w", err)
return fmt.Errorf("compiling %q test pipelines: %w", t.Package, err)
}

// Append anything the main package test needs.
Expand All @@ -126,7 +126,7 @@ func (b *Build) Compile(ctx context.Context) error {
}

if err := c.CompilePipelines(ctx, sm, cfg.Pipeline); err != nil {
return fmt.Errorf("compiling main pipelines: %w", err)
return fmt.Errorf("compiling %q pipelines: %w", cfg.Package.Name, err)
}

for i, sp := range cfg.Subpackages {
Expand All @@ -135,7 +135,7 @@ func (b *Build) Compile(ctx context.Context) error {
if sp.If != "" {
sp.If, err = util.MutateAndQuoteStringFromMap(sm.Substitutions, sp.If)
if err != nil {
return fmt.Errorf("mutating subpackage if: %w", err)
return fmt.Errorf("mutating subpackage %q, if: %w", sp.Name, err)
}
}

Expand Down Expand Up @@ -172,7 +172,7 @@ func (b *Build) Compile(ctx context.Context) error {
}

if err := tc.CompilePipelines(ctx, sm, cfg.Test.Pipeline); err != nil {
return fmt.Errorf("compiling main test pipelines: %w", err)
return fmt.Errorf("compiling %q test pipelines: %w", cfg.Package.Name, err)
}

te := &b.Configuration.Test.Environment.Contents
Expand Down Expand Up @@ -370,6 +370,27 @@ func (c *Compiled) gatherDeps(ctx context.Context, pipeline *config.Pipeline) er
return nil
}

func maybeIncludeSyntaxError(runs string, err error) error {
var perr syntax.ParseError
if !errors.As(err, &perr) {
return err
}

line := perr.Pos.Line()
lines := strings.Split(runs, "\n")
if line <= 0 || line > uint(len(lines)) {
return err
}

padding := len("> ") + int(perr.Pos.Col())

// For example...
// 14:13: not a valid test operator: -m
// > if [[ uname -m == 'x86_64']]; then
// ^
return fmt.Errorf("%w:\n> %s\n%*s", err, lines[line-1], padding, "^")
}

func stripComments(runs string) (string, error) {
parser := syntax.NewParser(syntax.KeepComments(false))
printer := syntax.NewPrinter()
Expand All @@ -391,7 +412,7 @@ func stripComments(runs string) (string, error) {
builder.WriteRune('\n')
return perr == nil
}); err != nil || perr != nil {
return "", errors.Join(err, perr)
return "", maybeIncludeSyntaxError(runs, errors.Join(err, perr))
}

return builder.String(), nil
Expand Down
11 changes: 11 additions & 0 deletions pkg/build/compile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,4 +147,15 @@ func Test_stripComments(t *testing.T) {
}
})
}

wantErr := `1:13: not a valid test operator: -m:
> if [[ uname -m == 'x86_64']]; then
^`

got, err := stripComments("if [[ uname -m == 'x86_64']]; then")
if err == nil {
t.Errorf("expected error, got %q", got)
} else if err.Error() != wantErr {
t.Errorf("want:\n%s\ngot:\n%s", wantErr, err)
}
}
2 changes: 1 addition & 1 deletion pkg/build/test.go
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ func (t *Test) TestPackage(ctx context.Context) error {

log.Infof("evaluating pipelines for package requirements")
if err := t.Compile(ctx); err != nil {
return fmt.Errorf("compiling test pipelines: %w", err)
return fmt.Errorf("compiling %s tests: %w", t.ConfigFile, err)
}

if t.Runner.Name() == container.QemuName {
Expand Down
38 changes: 37 additions & 1 deletion pkg/cli/compile.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (

apko_types "chainguard.dev/apko/pkg/build/types"
"chainguard.dev/melange/pkg/build"
"github.com/chainguard-dev/clog"
"github.com/spf13/cobra"
"go.opentelemetry.io/otel"
)
Expand Down Expand Up @@ -63,6 +64,9 @@ func compile() *cobra.Command {
var cpu, memory string
var timeout time.Duration
var extraPackages []string
var configFileGitCommit string
var configFileGitRepoURL string
var configFileLicense string

cmd := &cobra.Command{
Use: "compile",
Expand All @@ -72,6 +76,31 @@ func compile() *cobra.Command {
Args: cobra.MinimumNArgs(0),
RunE: func(cmd *cobra.Command, args []string) error {
ctx := cmd.Context()
log := clog.FromContext(ctx)

var buildConfigFilePath string
if len(args) > 0 {
buildConfigFilePath = args[0] // e.g. "crane.yaml"
}

// Favor explicit, user-provided information for the git provenance of the
// melange build definition. As a fallback, detect this from local git state.
// Git auto-detection should be "best effort" and not fail the build if it
// fails.
if configFileGitCommit == "" {
log.Infof("git commit for build config not provided, attempting to detect automatically")
commit, err := detectGitHead(ctx, buildConfigFilePath)
if err != nil {
log.Warnf("unable to detect commit for build config file: %v", err)
configFileGitCommit = "unknown"
} else {
configFileGitCommit = commit
}
}
if configFileGitRepoURL == "" {
log.Warnf("git repository URL for build config not provided")
configFileGitRepoURL = "https://unknown/unknown/unknown"
}

arch := apko_types.ParseArchitecture(archstr)
options := []build.Option{
Expand Down Expand Up @@ -108,6 +137,9 @@ func compile() *cobra.Command {
build.WithCPU(cpu),
build.WithMemory(memory),
build.WithTimeout(timeout),
build.WithConfigFileRepositoryCommit(configFileGitCommit),
build.WithConfigFileRepositoryURL(configFileGitRepoURL),
build.WithConfigFileLicense(configFileLicense),
}

if len(args) > 0 {
Expand Down Expand Up @@ -176,6 +208,10 @@ func compile() *cobra.Command {
cmd.Flags().StringVar(&memory, "memory", "", "default memory resources to use for builds")
cmd.Flags().DurationVar(&timeout, "timeout", 0, "default timeout for builds")

cmd.Flags().StringVar(&configFileGitCommit, "git-commit", "", "commit hash of the git repository containing the build config file (defaults to detecting HEAD)")
cmd.Flags().StringVar(&configFileGitRepoURL, "git-repo-url", "", "URL of the git repository containing the build config file (defaults to detecting from configured git remotes)")
cmd.Flags().StringVar(&configFileLicense, "license", "NOASSERTION", "license to use for the build config file itself")

return cmd
}

Expand All @@ -191,7 +227,7 @@ func CompileCmd(ctx context.Context, opts ...build.Option) error {
defer bc.Close(ctx)

if err := bc.Compile(ctx); err != nil {
return fmt.Errorf("failed to compile package: %w", err)
return fmt.Errorf("failed to compile %s: %w", bc.ConfigFile, err)
}

return json.NewEncoder(os.Stdout).Encode(bc.Configuration)
Expand Down
Loading