Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive source map support to error handling and LLB definitions in the dalec system. When errors occur, they now carry detailed location information about where in the spec file the error occurred, making debugging much easier for users.
- Implements custom UnmarshalYAML methods for types that need location tracking
- Updates package dependency handling to use maps instead of string slices for better type safety and source mapping
- Refactors test infrastructure to use a dedicated test runner with structured error reporting
Reviewed Changes
Copilot reviewed 54 out of 55 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sourcemap.go | New file implementing source map tracking and location constraint utilities |
| load.go | Refactored YAML loading to support source map context and AST-based unmarshaling |
| tests.go | Added source map tracking to test structures and error reporting |
| deps.go | Changed package dependencies from string slices to maps with constraint support |
| spec.go | Updated core spec types to support source mapping during unmarshaling |
| Multiple source files | Added UnmarshalYAML methods and source map fields to various source types |
| Multiple target files | Updated build systems to use new dependency structure and source location tracking |
| frontend/test_runner.go | Complete rewrite to use structured test runner with better error attribution |
cpuguy83
force-pushed
the
source_maps
branch
14 times, most recently
from
b74aa9c to
225e1e8
Compare
Merged
MorrisLaw
reviewed
Oct 7, 2025
| @@ -130,6 +122,9 @@ func (w *controlWrapper) depends(buf *strings.Builder, depsSpec *dalec.PackageDe | |||
| needsClone = false | |||
| } | |||
|
|
|||
Contributor
There was a problem hiding this comment.
super nit: extra whitespace that makes it not match the similar block below
Suggested change
| if !slices.ContainsFunc(deps, hasRust) { | ||
| return llb.Scratch(), errors.New("spec contains go modules but does not have golang in build deps") | ||
| if !slices.ContainsFunc(pkgNames, hasRust) { | ||
| return llb.Scratch(), errors.New("spec contains cargo homes but does not have rust in build deps") |
Merged
This makes it so the test evaluator runs *inside* the container. This means we don't need to do crazy things with stdio routing nor do we need to go through the buidlkit API for every file request. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This makes it so when an error is returned the error carries with it details about where in the spec file the error occurred. To accomplish, each object that wants to provide detailed location info needs to implement a custom UnmarshalYAML method which takes the location from the yaml AST and stores it so it can be retrieved later as needed. Future work here would involve providing even more detailed source mappings in some cases (e.g. a source type that has a "digest" field, which is a type we don't own so can't just store the mapping to it easily like we do for owned types). Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Really this shouldn't be needed since we aren't unmarshalling json here (ever), but its easier to fix this than to figure out something for the linter. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Some cases are just logic cleanups, others are fixing potential panics after some recent changes to the underlying type. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
MorrisLaw
approved these changes
Oct 17, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add source maps to errors and LLB definitions
This makes it so when an error is returned the error carries with it
details about where in the spec file the error occurred.
To accomplish, each object that wants to provide detailed location info
needs to implement a custom UnmarshalYAML method which takes the
location from the yaml AST and stores it so it can be retrieved later as
needed.
Future work here would involve providing even more detailed source
mappings in some cases (e.g. a source type that has a "digest" field,
which is a type we don't own so can't just store the mapping to it
easily like we do for owned types).
Closes #756 (includes the changes from there already, and some additional ones).