Skip to content

Commit

Permalink
Don't treat archive file differently from other files
Browse files Browse the repository at this point in the history
  • Loading branch information
@RomainMuller
RomainMuller committed Jan 16, 2025
1 parent 67e38ae commit 122037e
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 90 deletions.
66 changes: 26 additions & 40 deletions internal/jobserver/nbt/nbt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,9 @@ type (
onDone func() // Called once the original task has completed
done <-chan struct{} // Blocks until the original task has completed

isDone atomic.Bool // Whether the original task has completed yet.
archive string // Path to the archive produced by the original task. Available once done.
additionalFiles map[Label]string // Additional files produced by the original task. Available once done.
error error // Error from the original task. Available once done.
isDone atomic.Bool // Whether the original task has completed yet.
files map[Label]string // Additional files produced by the original task. Available once done.
error error // Error from the original task. Available once done.
}
)

Expand Down Expand Up @@ -103,21 +102,21 @@ type (
// the job server about the outcome of the build. It cannot be blank unless
// [*StartResponse.ArchivePath] is not blank.
FinishToken string `json:"token,omitempty"`
// ArchivePath is the path of the archive produced for the same
// [StartRequest.ImportPath] by another task. That archive must be re-used
// instead of creating a new one. It cannot be blank unless
// Files is the set of files produced by the original task, by label. These
// files must be re-used instead of re-created. Always blank if
// [*StartResponse.FinishToken] is not blank.
ArchivePath string `json:"archivePath,omitempty"`
// AdditionalFiles is the set of additional files produced by the original
// task, by label. These files must be re-used instead of re-created. Always
// blank if [*StartResponse.FinishToken] is not blank.
AdditionalFiles map[Label]string `json:"extra,omitempty"`
Files map[Label]string `json:"extra,omitempty"`
}
// Label is a label identifying an additional object produced by a task. It
// must be a valid part of a file name.
Label string
)

const (
LabelArchive Label = "_pkg_.a"
LabelAsmhdr Label = "go_asm.h"
)

func (StartRequest) Subject() string { return startSubject }
func (*StartResponse) IsResponseTo(StartRequest) {}

Expand Down Expand Up @@ -149,13 +148,13 @@ func (s *service) start(ctx context.Context, req StartRequest) (*StartResponse,
return nil, state.error
}

if state.archive == "" {
if len(state.files) == 0 {
// The context has expired or the upstream context has been canceled.
// We'll return this as [context.Canceled] either way.
return nil, context.Canceled
}

return &StartResponse{ArchivePath: state.archive, AdditionalFiles: state.additionalFiles}, nil
return &StartResponse{Files: state.files}, nil
}

// Otherwise, return a finalization token, etc...
Expand All @@ -172,15 +171,9 @@ type (
// FinishToken is forwarded from [*StartResponse.FinishToken], and cannot be
// blank.
FinishToken string `json:"token"`
// ArchivePath is the path to the archive produced by the compilation task.
// It must not be blank unless [FinishRequest.Error] is not nil. If present,
// the file must exist on disk, and will be hard-linked (or copied) into a
// temporary directory so it can be re-used by subsequent [StartRequest] for
// the same [FinishRequest.ImportPath].
ArchivePath string `json:"archivePath,omitempty"`
// AdditionalFiles is a list of additional files produced by the compilation
// task, associated to a user-defined label.
AdditionalFiles map[Label]string `json:"extra,omitempty"`
// Files is a list of files produced by the compilation task, associated to
// a user-defined label.
Files map[Label]string `json:"extra,omitempty"`
// Error is the error that occurred as a result of this compilation task, if
// any.
Error *string `json:"error,omitempty"`
Expand All @@ -193,7 +186,7 @@ type (
func (FinishRequest) Subject() string { return finishSubject }
func (*FinishResponse) IsResponseTo(FinishRequest) {}

var errNoArchiveNorError = errors.New("missing archive path, and no error reported")
var errNoFilesNorError = errors.New("missing files, and no error reported")

func (s *service) finish(ctx context.Context, req FinishRequest) (*FinishResponse, error) {
if req.ImportPath == "" || req.FinishToken == "" {
Expand Down Expand Up @@ -226,8 +219,7 @@ func (s *service) finish(ctx context.Context, req FinishRequest) (*FinishRespons

defer state.onDone()
log.Debug().
Str("archive-path", req.ArchivePath).
Any("additional-files", req.AdditionalFiles).
Any("files", req.Files).
Any("error", req.Error).
Msg("Compile task finished")

Expand All @@ -236,8 +228,8 @@ func (s *service) finish(ctx context.Context, req FinishRequest) (*FinishRespons
return &FinishResponse{}, nil
}

if req.ArchivePath == "" {
state.error = errNoArchiveNorError
if len(req.Files) == 0 {
state.error = errNoFilesNorError
return nil, state.error
}

Expand All @@ -247,26 +239,20 @@ func (s *service) finish(ctx context.Context, req FinishRequest) (*FinishRespons
return nil, state.error
}

state.additionalFiles = make(map[Label]string, len(req.AdditionalFiles))
for label, path := range req.AdditionalFiles {
state.files = make(map[Label]string, len(req.Files))
for label, path := range req.Files {
filename := filepath.Join(dir, string(label))
if err := files.Copy(ctx, path, filename); err != nil {
state.additionalFiles = nil
state.files = nil
state.error = fmt.Errorf("persisting additional file %q (%q): %w", path, label, err)
return nil, state.error
}
state.additionalFiles[label] = filename
}

state.archive = filepath.Join(dir, filepath.Base(req.ArchivePath))
if err := files.Copy(ctx, req.ArchivePath, state.archive); err != nil {
state.additionalFiles = nil
state.archive = ""
state.error = fmt.Errorf("persisting archive %q: %w", req.ArchivePath, err)
return nil, state.error
state.files[label] = filename
}

return &FinishResponse{}, nil
}

// ns is an arbitrary UUID used as a namespace for hashing import paths when storing artifacts in
// the temporary storage location.
var ns = uuid.MustParse("4BFB6F4B-212C-43A0-A581-A29C8B3D3BE4")
56 changes: 28 additions & 28 deletions internal/jobserver/nbt/nbt_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func Test(t *testing.T) {
start, err := subject.start(ctx, StartRequest{ImportPath: importPath, BuildID: buildID})
require.NoError(t, err)
require.NotEmpty(t, start.FinishToken)
assert.Empty(t, start.ArchivePath)
assert.Empty(t, start.Files)

archiveContent := uuid.NewString()
extraFileContent := uuid.NewString()
Expand All @@ -57,15 +57,17 @@ func Test(t *testing.T) {
res, err := subject.start(ctx, StartRequest{ImportPath: importPath, BuildID: buildID})
assert.NoError(t, err)
assert.Empty(t, res.FinishToken)
assert.NotEmpty(t, res.ArchivePath)
assert.NotEmpty(t, res.Files)
assert.Len(t, res.Files, 2)

content, err := os.ReadFile(res.ArchivePath)
path, ok := res.Files[LabelArchive]
assert.True(t, ok, "no file returned for %s", LabelArchive)
content, err := os.ReadFile(path)
assert.NoError(t, err)
assert.Equal(t, archiveContent, string(content))

assert.Len(t, res.AdditionalFiles, 1)
path, ok := res.AdditionalFiles[label]
assert.True(t, ok)
path, ok = res.Files[label]
assert.True(t, ok, "no file returned for %s", label)
content, err = os.ReadFile(path)
assert.NoError(t, err)
assert.Equal(t, extraFileContent, string(content))
Expand All @@ -79,10 +81,9 @@ func Test(t *testing.T) {
require.NoError(t, os.WriteFile(extraFile, []byte(extraFileContent), 0o644))

res, err := subject.finish(ctx, FinishRequest{
ImportPath: importPath,
FinishToken: start.FinishToken,
ArchivePath: archive,
AdditionalFiles: map[Label]string{label: extraFile},
ImportPath: importPath,
FinishToken: start.FinishToken,
Files: map[Label]string{LabelArchive: archive, label: extraFile},
})
require.NoError(t, err)
require.NotNil(t, res)
Expand All @@ -94,7 +95,7 @@ func Test(t *testing.T) {
start, err := subject.start(ctx, StartRequest{ImportPath: importPath, BuildID: buildID})
require.NoError(t, err)
require.NotEmpty(t, start.FinishToken)
assert.Empty(t, start.ArchivePath)
assert.Empty(t, start.Files)

archiveContent := uuid.NewString()

Expand All @@ -117,7 +118,7 @@ func Test(t *testing.T) {
res, err := subject.finish(ctx, FinishRequest{
ImportPath: importPath,
FinishToken: start.FinishToken,
ArchivePath: archive,
Files: map[Label]string{LabelArchive: archive},
})
require.NoError(t, err)
require.NotNil(t, res)
Expand All @@ -130,7 +131,7 @@ func Test(t *testing.T) {
start, err := subject.start(ctx, StartRequest{ImportPath: importPath, BuildID: buildID})
require.NoError(t, err)
require.NotEmpty(t, start.FinishToken)
assert.Empty(t, start.ArchivePath)
assert.Empty(t, start.Files)

archiveContent := uuid.NewString()
archive := filepath.Join(t.TempDir(), "_pkg_.a")
Expand All @@ -140,7 +141,7 @@ func Test(t *testing.T) {
res, err := subject.finish(ctx, FinishRequest{
ImportPath: importPath,
FinishToken: start.FinishToken,
ArchivePath: archive,
Files: map[Label]string{LabelArchive: archive},
})
require.NoError(t, err)
require.NotNil(t, res)
Expand All @@ -154,7 +155,7 @@ func Test(t *testing.T) {
start, err := subject.start(ctx, StartRequest{ImportPath: importPath, BuildID: buildID})
require.NoError(t, err)
require.NotEmpty(t, start.FinishToken)
assert.Empty(t, start.ArchivePath)
assert.Empty(t, start.Files)

archiveContent := uuid.NewString()
archive := filepath.Join(t.TempDir(), "_pkg_.a")
Expand All @@ -164,7 +165,7 @@ func Test(t *testing.T) {
res, err := subject.finish(ctx, FinishRequest{
ImportPath: importPath,
FinishToken: uuid.NewString(),
ArchivePath: archive,
Files: map[Label]string{LabelArchive: archive},
})
require.Error(t, err, "invalid finish token")
require.Nil(t, res)
Expand All @@ -173,7 +174,7 @@ func Test(t *testing.T) {
res, err := subject.finish(ctx, FinishRequest{
ImportPath: importPath,
FinishToken: start.FinishToken,
ArchivePath: archive,
Files: map[Label]string{LabelArchive: archive},
})
require.NoError(t, err)
require.NotNil(t, res)
Expand All @@ -186,7 +187,7 @@ func Test(t *testing.T) {
start, err := subject.start(ctx, StartRequest{ImportPath: importPath, BuildID: buildID})
require.NoError(t, err)
require.NotEmpty(t, start.FinishToken)
assert.Empty(t, start.ArchivePath)
assert.Empty(t, start.Files)

errorText := "simulated failure"

Expand Down Expand Up @@ -219,7 +220,7 @@ func Test(t *testing.T) {
start, err := subject.start(ctx, StartRequest{ImportPath: importPath, BuildID: buildID})
require.NoError(t, err)
require.NotEmpty(t, start.FinishToken)
assert.Empty(t, start.ArchivePath)
assert.Empty(t, start.Files)

var wg sync.WaitGroup
defer wg.Wait()
Expand All @@ -229,7 +230,7 @@ func Test(t *testing.T) {
defer wg.Done()

res, err := subject.start(ctx, StartRequest{ImportPath: importPath, BuildID: buildID})
assert.ErrorContains(t, err, errNoArchiveNorError.Error())
assert.ErrorContains(t, err, errNoFilesNorError.Error())
assert.Nil(t, res)
}()
}
Expand All @@ -238,7 +239,7 @@ func Test(t *testing.T) {
ImportPath: importPath,
FinishToken: start.FinishToken,
})
require.ErrorIs(t, err, errNoArchiveNorError)
require.ErrorIs(t, err, errNoFilesNorError)
require.Nil(t, res)
})

Expand All @@ -249,7 +250,7 @@ func Test(t *testing.T) {
start, err := subject.start(ctx, StartRequest{ImportPath: importPath, BuildID: buildID})
require.NoError(t, err)
require.NotEmpty(t, start.FinishToken)
assert.Empty(t, start.ArchivePath)
assert.Empty(t, start.Files)

// Deliberately non-existent!
archive := filepath.Join(t.TempDir(), "deliberately-missing", "_pkg_.a")
Expand All @@ -270,7 +271,7 @@ func Test(t *testing.T) {
res, err := subject.finish(ctx, FinishRequest{
ImportPath: importPath,
FinishToken: start.FinishToken,
ArchivePath: archive,
Files: map[Label]string{LabelArchive: archive},
})
require.ErrorContains(t, err, archive)
require.Nil(t, res)
Expand All @@ -283,7 +284,7 @@ func Test(t *testing.T) {
start, err := subject.start(ctx, StartRequest{ImportPath: importPath, BuildID: buildID})
require.NoError(t, err)
require.NotEmpty(t, start.FinishToken)
assert.Empty(t, start.ArchivePath)
assert.Empty(t, start.Files)

label := Label(uuid.NewString())
// Deliberately non-existent!
Expand All @@ -306,10 +307,9 @@ func Test(t *testing.T) {
require.NoError(t, os.WriteFile(archive, []byte(uuid.NewString()), 0o644))

res, err := subject.finish(ctx, FinishRequest{
ImportPath: importPath,
FinishToken: start.FinishToken,
ArchivePath: archive,
AdditionalFiles: map[Label]string{label: extraFile},
ImportPath: importPath,
FinishToken: start.FinishToken,
Files: map[Label]string{LabelArchive: archive, label: extraFile},
})
require.ErrorContains(t, err, extraFile)
require.Nil(t, res)
Expand Down
Loading

0 comments on commit 122037e

Please sign in to comment.